Skip to content
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

DOC: examples for more commands #4091

Merged
merged 19 commits into from Jan 28, 2020
Merged

DOC: examples for more commands #4091

merged 19 commits into from Jan 28, 2020

Conversation

adswa
Copy link
Member

@adswa adswa commented Jan 27, 2020

  • I've added a few examples to update, uninstall, remove, download-url, diff, and run-procedure.
  • For drop, I removed duplicate examples that were still present in the docstring.
  • For rerun, I transferred examples from the docstring into the _examples_ list and added Python translations when possible.

I deemed one of the examples of rerun as not very suitable for the Python API help:

Re-execute all previous commands and compare the old and new results:
% # on master branch
% datalad rerun --branch=verify --since=
% # now on verify branch
% datalad diff --revision=master..
% git log --oneline --left-right --cherry-pick master...

Therefore, I modified build_example to only include an example in the help text of an API if a code snippet exists for this API, and did not translate it to a Python example.

@mih
Copy link
Member

mih commented Jan 27, 2020

Test failure is unrelated and familiar.

Enables to simply not have such an example key, instead of being
forced to provide an empty one.
mih
mih approved these changes Jan 27, 2020
Copy link
Member

@mih mih left a comment

Awesome, thanks much! I made some minor improvements to the code. Will push directly into this PR.

code_cmd="datalad rerun --onto= --since=HEAD~5"),
dict(text="""Re-execute all previous commands and compare the old and
new results""",
code_py=" ",
Copy link
Member

@mih mih Jan 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO it would be cleaner to not have this key, if there is no example. Implemented support for it.

Copy link
Member Author

@adswa adswa Jan 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

% datalad rerun --branch=verify --since=
% # now on verify branch
% datalad diff --revision=master..
% git log --oneline --left-right --cherry-pick master..."""),
Copy link
Member

@mih mih Jan 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It rendered like this:

image

I implemented a mode, where no custom prefixing is done, if there already is a prefix in the example

image

Copy link
Member Author

@adswa adswa Jan 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching that!

Copy link
Collaborator

@kyleam kyleam left a comment

Thanks for working on this.

I didn't do a thorough check, but here are some quick comments for glancing through these.

datalad/core/local/diff.py Outdated Show resolved Hide resolved
datalad/distribution/remove.py Outdated Show resolved Hide resolved
datalad/distribution/remove.py Outdated Show resolved Hide resolved
datalad/distribution/uninstall.py Outdated Show resolved Hide resolved
datalad/distribution/uninstall.py Outdated Show resolved Hide resolved
datalad/interface/download_url.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jan 27, 2020

Codecov Report

Merging #4091 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4091      +/-   ##
==========================================
+ Coverage   89.72%   89.72%   +<.01%     
==========================================
  Files         274      274              
  Lines       36744    36752       +8     
==========================================
+ Hits        32967    32975       +8     
  Misses       3777     3777
Impacted Files Coverage Δ
datalad/core/local/run.py 98.7% <ø> (ø) ⬆️
datalad/distribution/get.py 88.78% <ø> (ø) ⬆️
datalad/core/local/status.py 98.03% <ø> (ø) ⬆️
datalad/core/local/create.py 94.73% <ø> (ø) ⬆️
datalad/distribution/drop.py 98.79% <ø> (ø) ⬆️
datalad/core/local/save.py 98.68% <ø> (ø) ⬆️
datalad/interface/rerun.py 96.4% <100%> (+0.01%) ⬆️
datalad/distribution/uninstall.py 100% <100%> (ø) ⬆️
datalad/core/local/diff.py 96.2% <100%> (+0.04%) ⬆️
datalad/interface/base.py 90.66% <100%> (+0.05%) ⬆️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c741a5b...f5bb3eb. Read the comment docs.

kyleam added a commit to kyleam/datalad that referenced this pull request Jan 27, 2020
Our help text has lots of cases of where the first word in a line is
incorrectly capitalized when the line ends with a colon.  The code
that does this has a comment that says this is to capitalize the first
word of all "headings", but it's not clear what those headings are.
Without this code, the help output of all of our current commands only
differ in cases of incorrect capitalization, so it looks like this
code is no longer serving its original purpose but causing lots of
errors.

Drop it.

Re: datalad#4091 (comment)
kyleam added a commit to kyleam/datalad that referenced this pull request Jan 28, 2020
Our help text has lots of spots where the first word in a line is
incorrectly capitalized when the line ends with a colon.  The code
that does this has a comment that says this is to capitalize the first
word of all "headings", but it's not clear what those headings are.

Without this code, the help output of all of our current commands only
differs in cases of incorrect capitalization, so it looks like this
code is no longer serving its original purpose while causing issues
elsewhere.  Drop it.

Re: datalad#4091 (comment)
adswa and others added 4 commits Jan 28, 2020
5260683 (DOC: use dataset argument in Python example if CMD example
has it too) added a `dataset` argument to several Python examples that
use a bound dataset method.  This would raise an exception: the
dataset is already being specified through the bound method.

As discussed in dataladgh-4091 [*], it might make sense to use unbound
methods consistently in all Python API examples, but until will make
that switch, remove the `dataset` argument from these spots.

[*]: datalad#4091 (comment)
kyleam
kyleam approved these changes Jan 28, 2020
Copy link
Collaborator

@kyleam kyleam left a comment

@adswa Thanks for the update. I pushed a fix on top.

kyleam added a commit that referenced this pull request Jan 28, 2020
@kyleam kyleam merged commit f5bb3eb into datalad:master Jan 28, 2020
kyleam added a commit to kyleam/datalad that referenced this pull request Jan 30, 2020
The Python examples have a mix of dataset-bound method calls and
unbound calls (i.e., calls to datalad.api functions).  While the
dataset-bound calls are useful in general, using unbound calls in the
examples has the following benefits:

  * There are fewer unrepresented steps.

    The snippets aren't complete examples; they don't have setup code
    and the paths are placeholders.  With both unbound and bound
    methods, there are implied imports.  With the bound methods, there
    are additional missing pieces regarding what `ds` is, how the
    dataset was created, and what path it points to.

  * Unbound methods share the command-line path handling logic.

    A dataset string sent in through an unbound command should trigger
    the same relative path handling as the command line.  Using the
    unbound methods results in snippet pairs that are a closer match.
    The only assumption necessary is that the snippets are executed
    from the same current working directory.

On the other hand, a downside of _not_ using bound methods in the
examples is that users won't learn that bound methods exist through
the examples.  But snippets probably aren't a good means of
demonstrating this anyway, at least with their current capabilities.

Convert all the examples to unbound calls.  Note that some of the
converted status() examples add an explicit `dataset` argument either
because it's required for the intended behavior or because it's
helpful for comparison with neighboring examples.

Re: datalad#4091 (comment)
@adswa adswa deleted the ENH/examples branch Dec 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants