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

RF: throw dedicated RuntimeError with information about question in case of SilentConsoleLog #4553

Merged
merged 2 commits into from May 20, 2020

Conversation

yarikoptic
Copy link
Member

@yarikoptic yarikoptic commented May 19, 2020

It is hard to impossible to troubleshoot failures of datalad in automated
deployments where we want to ask some question for some reason, but UI simply
does not support that since it is rightfully silent/non-interactive.

See e.g. templateflow/templateflow#50 .

With this PR we would still fail but with a little more informative RuntimeError,
instead of obscure AttributeError stating what was actually asked. We will not
disclose possible present other fields other than question and title if it was a
question asking for secret information ("hidden"). AFAIK in our use of the dialogues
we do not disclose any sensitive information in title or the question but I could be
proven wrong.

I did position this one on top of maint since might benefit people using released
version in production. Delaying till 0.13.0 would require them to migrate first to
just be able to troubleshoot

…ase of SilentConsoleLog

It is hard to impossible to troubleshoot failures of datalad in automated
deployments where we want to ask some question for some reason, but UI simply
does not support that since it is rightfully silent/non-interactive.

See e.g. templateflow/templateflow#50 .

With this PR we would still fail but with a little more informative RuntimeError,
instead of obscure AttributeError stating what was actually asked.  We will not
disclose possible present other fields other than question and title if it was a
question asking for secret information ("hidden").  AFAIK in our use of the dialogues
we do not disclose any sensitive information in title or the question but I could be
proven wrong.

I did position this one on top of maint since might benefit people using released
version in production.  Delaying till 0.13.0 would require them to migrate first to
just be able to troubleshoot
@yarikoptic yarikoptic added enhancement UX DX labels May 19, 2020
Copy link
Contributor

@effigies effigies left a comment

Minor grammatical suggestion. Also, would newlines be better separators than spaces between question, title and args? It's probably parseable as-is, but I'm wondering if longer questions or more arguments might not get too cluttered.

datalad/ui/dialog.py Outdated Show resolved Hide resolved
Co-authored-by: Chris Markiewicz <effigies@gmail.com>
@yarikoptic
Copy link
Member Author

@yarikoptic yarikoptic commented May 19, 2020

Also, would newlines be better separators than spaces between question, title and args? It's probably parseable as-is, but I'm wondering if longer questions or more arguments might not get too cluttered.

I don't know why really, but for some reason I am avoiding adding multiline formatting into exception messages... I wonder if there is some reason for such behavior of mine since those messages are indeed just for a user to see and I could have gone "wild" in formatting them to actually make them readable. hm...

@yarikoptic
Copy link
Member Author

@yarikoptic yarikoptic commented May 19, 2020

Probably for such cases I better come up with some dedicated (probably mix-in) exception class which would get args, kwargs args and then render them nicely in its str. Then it could be reused for a number of use-cases

@effigies
Copy link
Contributor

@effigies effigies commented May 20, 2020

Well, don't hold this up on my hypothetical aesthetics. If it turns out to be unreadable in practice, that's what the "New pull request" button is for.

@yarikoptic
Copy link
Member Author

@yarikoptic yarikoptic commented May 20, 2020

Oh well, while liberal Germans are vacationing, the anal Russian might be looking for the ultimate perfection ;-)

@codecov
Copy link

@codecov codecov bot commented May 20, 2020

Codecov Report

Merging #4553 into master will decrease coverage by 0.48%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4553      +/-   ##
==========================================
- Coverage   90.14%   89.66%   -0.49%     
==========================================
  Files         275      285      +10     
  Lines       37101    38624    +1523     
==========================================
+ Hits        33446    34633    +1187     
- Misses       3655     3991     +336     
Impacted Files Coverage Δ
datalad/ui/dialog.py 89.44% <100.00%> (+0.68%) ⬆️
datalad/ui/tests/test_dialog.py 98.98% <100.00%> (+0.10%) ⬆️
datalad/tests/test_direct_mode.py 48.78% <0.00%> (-12.34%) ⬇️
datalad/interface/annotate_paths.py 90.18% <0.00%> (-4.37%) ⬇️
datalad/tests/test_s3.py 47.61% <0.00%> (-3.50%) ⬇️
datalad/core/distributed/clone.py 90.34% <0.00%> (-3.10%) ⬇️
datalad/metadata/extractors/tests/test_xmp.py 88.00% <0.00%> (-2.63%) ⬇️
datalad/metadata/extractors/tests/test_image.py 88.00% <0.00%> (-2.63%) ⬇️
datalad/distribution/tests/test_update.py 97.81% <0.00%> (-2.19%) ⬇️
datalad/metadata/extractors/tests/test_exif.py 91.66% <0.00%> (-1.89%) ⬇️
... and 151 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 bce4caf...8bfe827. Read the comment docs.

datalad/ui/dialog.py Show resolved Hide resolved
@kyleam
Copy link
Contributor

@kyleam kyleam commented May 20, 2020

I did position this one on top of maint since might benefit people using released
version in production. Delaying till 0.13.0 would require them to migrate first to
just be able to troubleshoot

I just merged this to maint, but it looks like you chose master as the base for the PR. Sorry I didn't spot it, but based on the above I'm pretty sure you intended to choose maint. Rather than closing this or trying to change the base now, perhaps let's just wait for maint to hit master, so this shows up as merged.

@kyleam kyleam merged commit 4ca369b into datalad:master May 20, 2020
11 of 12 checks passed
@kyleam
Copy link
Contributor

@kyleam kyleam commented May 20, 2020

@effigies:

Well, don't hold this up on my hypothetical aesthetics. If it turns out to be unreadable in practice, that's what the "New pull request" button is for.

@yarikoptic:

Oh well, while liberal Germans are vacationing, the anal Russian might be looking for the ultimate perfection ;-)

Oy, I'm sorry, I overlooked that @yarikoptic was intending additional improvements for this series.

@effigies
Copy link
Contributor

@effigies effigies commented May 20, 2020

The enterprising American disrupts the process and makes history.

@yarikoptic
Copy link
Member Author

@yarikoptic yarikoptic commented May 20, 2020

;-) LOL, indeed! so I will just succumb to the progress and will avoid being anal -- I think it is + and + ;)

@yarikoptic yarikoptic deleted the enh-silent-console-log branch May 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DX enhancement UX
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants