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: make warning on absent annex appear less like an error #4323

Merged
merged 1 commit into from Mar 19, 2020

Conversation

adswa
Copy link
Member

@adswa adswa commented Mar 19, 2020

I have reworded the warning and info messages related to adding siblings without an annex. Mostly, I've removed the word "failed". I've played around with "downgrading" the message from "warning" to "info", but I guess the color coding as a warning helps to not miss a problem should there ever be one. The text should make sure that this Warning is no need for worry if the remote is a pure Git remote.

I think its better than the previous messages, but I'm not yet 100% satisfied. Here is how the message looks like for the situation described in #4322

datalad siblings add --name bitbucket --url https://testing_ittacc@bitbucket.org/testing_ittacc/mydataset.git
[INFO   ] Could not enable annex remote bitbucket. This is expected if bitbucket is a pure Git remote, or happens if it is not accessible. 
[WARNING] Could not detect whether bitbucket carries an annex. If bitbucket is a pure Git remote, this is expected. Remote was marked by annex as annex-ignore. Edit .git/config to reset if you think that was done by mistake due to absent connection etc 
.: bitbucket(-) [https://testing_ittacc@bitbucket.org/testing_ittacc/mydataset.git (git)]

If anyone has other suggestions, please say so. :)

This fixes #4322

@codecov
Copy link

@codecov codecov bot commented Mar 19, 2020

Codecov Report

Merging #4323 into master will increase coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4323      +/-   ##
==========================================
+ Coverage   88.83%   88.87%   +0.04%     
==========================================
  Files         285      285              
  Lines       37457    37457              
==========================================
+ Hits        33275    33291      +16     
+ Misses       4182     4166      -16     
Impacted Files Coverage Δ
datalad/distribution/siblings.py 76.27% <100.00%> (ø)
datalad/downloaders/tests/test_http.py 60.96% <0.00%> (+2.16%) ⬆️
datalad/downloaders/http.py 74.90% <0.00%> (+2.78%) ⬆️

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 659f70f...38b072d. Read the comment docs.

kyleam
kyleam approved these changes Mar 19, 2020
Copy link
Contributor

@kyleam kyleam left a comment

Thanks, I agree that these are improvements.

I've played around with "downgrading" the message from "warning" to "info", but I guess the color coding as a warning helps to not miss a problem should there ever be one. The text should make sure that this Warning is no need for worry if the remote is a pure Git remote.

I have a slight preference for an info message here, but the tradeoff you're mentioning does sound like a valid reason to stick to the warning.

@adswa
Copy link
Member Author

@adswa adswa commented Mar 19, 2020

Thanks for the review, @kyleam! Let's go for a small improvement (wording) now, and for a more radical change (warning versus info) if rewording feels like it did not go far enough after a while.

@adswa adswa merged commit 6ddb171 into datalad:master Mar 19, 2020
12 checks passed
@adswa adswa deleted the doc-sibling branch Mar 19, 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.

2 participants