-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add canonical aliases for push/pull #787
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
docs/README.md
Outdated
push Deprecated: Push new code to a kernel and run the kernel | ||
pull Deprecated: Pull down code from a kernel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you mention what the alternative is? I know it says so above, but for those who are already familiar, they might be scanning down the list and miss it.
docs/README.md
Outdated
push Deprecated: Push new code to a kernel and run the kernel | ||
pull Deprecated: Pull down code from a kernel | ||
output Get data output from the latest kernel runth new code and run it (formerly push) | ||
update Update a kernel wi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Documentation looks incomplete?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weird. The remainder of that sentence is on the previous line. Fixed.
docs/README.md
Outdated
##### Push a kernel | ||
|
||
##### Push a kernel (deprecated) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please mention alternative here (and below).
docs/README.md
Outdated
-t N, --timeout N Limit the run time of a kernel to the given number of seconds. | ||
The global maximum time will not be exceeded. | ||
-p FOLDER, --path FOLDER | ||
Folder for upload, containing data files and a special kernel-metadata.json file (https://github.com/Kaggle/kaggle-api/wiki/Kernel-Metadata). Defaults to current working directory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit - is this supposed to be on the next line? What about moving everything over a couple more spaces so it can start on the same line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was just one long line. I broke it up.
kaggle/cli.py
Outdated
'pull', | ||
formatter_class=argparse.RawTextHelpFormatter, | ||
help=Help.command_kernels_pull, | ||
aliases=['get'], # Could be 'read' but this is consistent with models |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW - Get
is the canonical verb per AIP, probably why they picked it.
##### Update a kernel | ||
|
||
``` | ||
usage: kaggle kernels update [-h] -p FOLDER |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
push
functions as an upsert rather than a pure update. Is update
really an upsert, or do we eventually want users to create
for the first version followed by update
for remaining versions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added extra text to clarify that update
follows get
.
Thanks for making this update even better! |
Using
get
rather thanread
as it is consistent with models.