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

exceptions: Restore CLI rendering of CommandError subclasses #4966

Merged
merged 4 commits into from Oct 2, 2020

Conversation

kyleam
Copy link
Contributor

@kyleam kyleam commented Sep 25, 2020

I misspelled a remote name when calling datalad get --source and saw this message, which by itself I'd take to mean a remote named 'annex get' is not available.

RemoteNotAvailableError: ''annex get''

That's a regression that came with v0.13.0 and affects all CommandError subclasses. The final commit of this series restores the display to

Remote 'DO-NOT-EXIST' is not available. Command failed:
RemoteNotAvailableError: 'annex get'

The three commits before that are cleanup commits in surrounding areas, and any of them can safely be dropped.

CommandError.to_str() (which was added in v0.13.0) forces the cmd
argument to a list and then feeds it through join_cmdline().  There
are callers that pass in a string, which leads to quoting like this:

    ''annex get''

Leave non-list commands as is to avoid this.
There are two spots that can raise a RemoteNotAvailableError for 'git
annex get' errors: AnnexRepo.get() and ._run_annex_command_json().
The latter uses 'git annex <subcommand>' (so 'git annex get', in this
case) for the command.

For consistency update AnnexRepo.get() to pass the same command to
RemoteNotAvailableError.
RemoteNotAvailableError is now used in gitrepo.py, and any discussion
of GitPython, which we no longer use, is of course irrelevant.
09d3fd8 (ENH: Prefer structured over rendered info, 2020-02-16,
dataladgh-4157) switched datalad.cmdline.main over to using to_str() for
rendering a caught CommandError.  None of the CommandError subclasses
have a to_str() method, though, so the value returned by
CommandError.to_str() is displayed rather than the value returned by
the child's __str__() method.

For example, now an OutOfSpaceError is shown as "OutOfSpaceError:  "
rather than "OutOfSpaceError:  needs N more".  Likewise, calling
`datalad get --source=DO-NOT-EXIST` no longer shows the remote name,
just

    RemoteNotAvailableError: 'annex get'

Convert the __str__() methods of all subclasses to to_str().  The
message above becomes

    Remote 'DO-NOT-EXIST' is not available. Command failed:
    RemoteNotAvailableError: 'annex get'

This commit just intends to restore the displayed information that was
lost with 09d3fd8.  However, the rendering of these CommandError
subclasses could arguably be improved (e.g., the "Command failed:
RemoteNotAvailableError: 'annex get'" stacking is confusing and could
probably be dropped altogether).
@codecov
Copy link

codecov bot commented Sep 25, 2020

Codecov Report

Merging #4966 into maint will decrease coverage by 0.07%.
The diff coverage is 77.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##            maint    #4966      +/-   ##
==========================================
- Coverage   89.47%   89.40%   -0.08%     
==========================================
  Files         289      289              
  Lines       40716    40717       +1     
==========================================
- Hits        36432    36402      -30     
- Misses       4284     4315      +31     
Impacted Files Coverage Δ
datalad/support/annexrepo.py 86.84% <ø> (ø)
datalad/support/exceptions.py 83.67% <77.77%> (+0.08%) ⬆️
datalad/downloaders/http.py 81.85% <0.00%> (-2.71%) ⬇️
datalad/interface/tests/test_unlock.py 95.87% <0.00%> (-2.07%) ⬇️
datalad/downloaders/tests/test_http.py 88.22% <0.00%> (-1.93%) ⬇️
datalad/tests/utils.py 86.25% <0.00%> (-0.55%) ⬇️
datalad/core/local/tests/test_save.py 97.51% <0.00%> (-0.46%) ⬇️
datalad/support/tests/test_annexrepo.py 96.34% <0.00%> (-0.39%) ⬇️
datalad/tests/test_utils.py 95.31% <0.00%> (-0.28%) ⬇️

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 3e03cfa...51b5767. Read the comment docs.

@yarikoptic
Copy link
Member

hm, at some point we should take some other approach to tune up __str__ rendering, rather than have ad-hoc as_str. Actually we could have probably just made __str__ to carry the keyword arg and invoke it with it whenever needed... Seems to work:

$> python pp.py
Buga None
Buga 42
(dev3) 1 26405 [1].....................................:Thu 01 Oct 2020 08:04:10 PM EDT:.
lena:/tmp
$> cat pp.py
class C:
   def __str__(self, kw=None):
      return "Buga %r" % kw

e = C()
print(str(e))
print(e.__str__(42))

But since maint let's keep it without such RFings. Thanks!

@yarikoptic yarikoptic merged commit 5697b16 into datalad:maint Oct 2, 2020
@kyleam kyleam deleted the command-error-subclass-tostr branch October 2, 2020 13:12
@yarikoptic yarikoptic added this to the 0.13.5 milestone Dec 12, 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

2 participants