Remove swagger #659
Remove swagger #659
Conversation
jplotts
left a comment
jplotts
left a comment
There was a problem hiding this comment.
I'll approve this, but fair warning that I was really only able to skim it. The PR is titled Remove swagger but there are other changes here that should probably have been split into their own PRs.
kaggle/api/kaggle_api_extended.py
Outdated
|
|
||
| try: | ||
| self.api_client = ApiClient(configuration) | ||
| self.api_client = None # ApiClient(configuration) |
There was a problem hiding this comment.
Is this intentional? If so, when is api_client filled in (or should it be removed)?
There was a problem hiding this comment.
Removed, thanks.
kaggle/api/kaggle_api_extended.py
Outdated
| data['title'] = model.title | ||
| data['subtitle'] = model.subtitle | ||
| data['isPrivate'] = model.isPrivate # TODO Add a test to ensure default is True | ||
| data[ |
There was a problem hiding this comment.
no action required, but line breaking due to a comment is kind of ugly. Maybe something to fix with the autoformatter some day (only if it's easy).
There was a problem hiding this comment.
Reworded to keep on one line.
kaggle/api/kaggle_api_extended.py
Outdated
| remote_size = int(response.headers['Content-Length']) | ||
| local_size = os.path.getsize(outfile) | ||
| if local_size < remote_size: | ||
| if local_size < remote_size or True: # DEBUG |
There was a problem hiding this comment.
Seems like this should be reverted?
There was a problem hiding this comment.
I don't really have a strong opinion, but this isn't something we meant to expose right now. I don't think it's harmful, but if there's an easy way to exclude it, that's probably better.
There was a problem hiding this comment.
I don't understand why this is included. Previously, kapigen ignored inbox and blobs. Both of those are required in kagglesdk. After I stopped ignoring them, this showed up, but it doesn't seem to match either.
b/384565864
|
|
||
| return self._client.call("kernels.KernelsApiService", "ApiGetKernelSessionStatus", request, ApiGetKernelSessionStatusResponse) | ||
|
|
||
| def download_kernel_output(self, request: ApiDownloadKernelOutputRequest = None) -> HttpRedirect: |
There was a problem hiding this comment.
similar comment as above; if we don't need this in Kaggle API, maybe it would be nice to exclude it.
There was a problem hiding this comment.
This is required by kaggle kernels output.
tools/GeneratePythonLibrary.sh
Outdated
|
|
||
| function generate-from-swagger { | ||
| java -jar ./tools/swagger-codegen-cli.jar generate -i $SWAGGER_YAML -c $SWAGGER_CONFIG -l python || true | ||
| # java -jar ./tools/swagger-codegen-cli.jar generate -i $SWAGGER_YAML -c $SWAGGER_CONFIG -l python || true |
There was a problem hiding this comment.
Might be nice to clean this up, both removing commented out code and rename generate-from-swagger if that's not what happening.
There was a problem hiding this comment.
Done, but I have additional work to do here.
tools/GeneratePythonLibrary.sh
Outdated
| kaggle/*.py-e \ | ||
| kaggle/api/*.py-e \ | ||
| kaggle/*.py.bak | ||
| # requirements.txt \ |
There was a problem hiding this comment.
Do we not need requirements anymore?
There was a problem hiding this comment.
This was actually removing the file. I want to keep it. No need for the comment, though.
|
3 participants
Reminders:
kagglesdkis generated bykapigen-- no review neededkaggledirectory is a copy ofsrc/kaggle-- no need to review both