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

BF: Add dashes to 'force' option in non-empty directory error message #6078

Merged
merged 1 commit into from Oct 26, 2021

Conversation

DisasterMo
Copy link
Contributor

Description

In my opinion all mentioned options should be displayed 'in full', i.e. with any and all dashes.

Copy link
Member

@mih mih left a comment

Choose a reason for hiding this comment

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

In principle yes! But now Python API users will be confused.

Grep for [CMD: and [PY:, there are macros that allow for alternative specification, based on API

@codecov
Copy link

codecov bot commented Oct 14, 2021

Codecov Report

Merging #6078 (0885f9b) into maint (62a8987) will decrease coverage by 0.08%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##            maint    #6078      +/-   ##
==========================================
- Coverage   90.24%   90.15%   -0.09%     
==========================================
  Files         312      312              
  Lines       42215    42215              
==========================================
- Hits        38096    38060      -36     
- Misses       4119     4155      +36     
Impacted Files Coverage Δ
datalad/core/local/create.py 97.79% <ø> (ø)
datalad/core/local/tests/test_create.py 100.00% <ø> (ø)
datalad/core/local/tests/test_results.py 92.85% <0.00%> (-7.15%) ⬇️
datalad/core/local/tests/test_diff.py 97.23% <0.00%> (-2.31%) ⬇️
datalad/core/distributed/clone.py 89.26% <0.00%> (-1.79%) ⬇️
datalad/runner/tests/test_witless_runner.py 93.61% <0.00%> (-1.42%) ⬇️
datalad/core/local/tests/test_status.py 97.32% <0.00%> (-0.90%) ⬇️
datalad/interface/utils.py 94.58% <0.00%> (-0.73%) ⬇️
datalad/tests/utils.py 88.57% <0.00%> (-0.65%) ⬇️
datalad/core/distributed/tests/test_push.py 98.30% <0.00%> (-0.64%) ⬇️
... and 3 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 62a8987...0885f9b. Read the comment docs.

@DisasterMo
Copy link
Contributor Author

I don't think I understand how to use this. It always prints out everything:

[will not create a dataset in a non-empty directory, use '[CMD: --force CMD][PY: force PY]' option to ignore]

@mih
Copy link
Member

mih commented Oct 15, 2021

Ah! Sorry! I misread the location where this is happening. The macro-handling I mentioned only runs for API doc generation. See

def alter_interface_docs_for_api(docs):
and
def alter_interface_docs_for_cmdline(docs):
.

I think it would be lovely to have the same feature for result/error messages -- which would require pulling this handling out of these functions into a more targeted helper.

However, right now I am not sure how we could tell whether a result renderer is called in the context of a cmdline invocation vs a Python session.

@yarikoptic
Copy link
Member

FWIW, on one hand I would vote to just merge this PR for now and tune up later whenever we have support for "discriminating" one invocation type from another since ATM those messages are shown to the users who are primarily operating from CLI.

But on the other, I think user can grasp that use 'force' option talks about an option which are typically provided as --option in CLI.

so here are my .5c to one and .5c to another ;)

@mih
Copy link
Member

mih commented Oct 26, 2021

OK, let me break the spell. We have tons of cmdline-like parameter mentions in the code base. More homogeneity towards this particular attractor is not a bad thing.

@mih mih added the semver-patch Increment the patch version when merged label Oct 26, 2021
@mih mih merged commit d8abfe5 into datalad:maint Oct 26, 2021
@adswa
Copy link
Member

adswa commented Oct 26, 2021

Late in commenting on this, but --force instead of force also matches what was done in #5194 for drop 👍

@mih mih modified the milestone: 0.16.0 Nov 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch Increment the patch version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants