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: make publish understand --since=^ and deprecate --since= #4683

Merged
merged 2 commits into from Sep 4, 2020

Conversation

yarikoptic
Copy link
Member

Positioned against maint since

Who knows, may be in 0.15 the entire publish would be gone...

@yarikoptic yarikoptic added the UX user experience label Jul 7, 2020
@kyleam
Copy link
Collaborator

kyleam commented Jul 8, 2020

I have no objections to making publish understand '^' for consistency. As for applying it to maint, all right, though I think we should avoid using "it doesn't break" (should be true for nearly all code for master too) and "improves things for me" as reasons to make non-bugfix changes to maint.

However, I really don't think we should introduce deprecations on maint unless we have a very strong reason to. In this case, I think publish should just learn to treat '^' as another spelling of ''. Continuing to support the latter has virtually no maintenance cost.

Who knows, may be in 0.15 the entire publish would be gone...

It shouldn't be. We haven't deprecated it yet, so that'd be at least 0.16.

@yarikoptic
Copy link
Member Author

Re no maintenance cost - there is UX cost due to more than one way to do the same, confusion etc. I wish I just did it for 0.13, where it would have been right in place.. heh heh... Also iirc now there is datalad diff which might need the same (didn't check)

@kyleam
Copy link
Collaborator

kyleam commented Jul 8, 2020

there is UX cost due to more than one way to do the same, confusion etc.

Yes, ideally an interface wouldn't have multiple ways to say the same thing. But in this case the tradeoff is that it allows consistency between publish and push without making users or us have to go through a deprecation sequence.

@yarikoptic
Copy link
Member Author

But in this case the tradeoff is that it allows consistency between publish and push without making users or us have to go through a deprecation sequence.

it wouldn't be "consistent" unless we also allow empty string for push... the sooner we encourage (and really weakly since that deprecation warning wouldn't be shown by default) users to adhere to consistent interface, the better IMHO.

@kyleam
Copy link
Collaborator

kyleam commented Jul 13, 2020

But in this case the tradeoff is that it allows consistency between publish and push without making users or us have to go through a deprecation sequence.

it wouldn't be "consistent" unless we also allow empty string for push...

To reword my statement in a less sloppy, context-dependent way: "it allows users to consistently say --since=^ across publish and push".

the sooner we encourage (and really weakly since that deprecation warning wouldn't be shown by default) users to adhere to consistent interface, the better IMHO.

I disagree. I'll leave it at that, since I'd just repeat my statements from above.

@yarikoptic
Copy link
Member Author

@bpoldrack @mih @adswa , any opinion? Deprecate or not --since= in publish?

@bpoldrack
Copy link
Member

I find @kyleam's argument convincing.

@yarikoptic
Copy link
Member Author

Removed deprecation.

@yarikoptic yarikoptic added merge-if-ok OP considers this work done, and requests review/merge and removed feedback-desired labels Sep 4, 2020
@codecov
Copy link

codecov bot commented Sep 4, 2020

Codecov Report

Merging #4683 into maint will decrease coverage by 0.27%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##            maint    #4683      +/-   ##
==========================================
- Coverage   89.92%   89.65%   -0.28%     
==========================================
  Files         288      288              
  Lines       40158    40359     +201     
==========================================
+ Hits        36114    36184      +70     
- Misses       4044     4175     +131     
Impacted Files Coverage Δ
datalad/distribution/publish.py 88.81% <100.00%> (ø)
datalad/distribution/tests/test_publish.py 100.00% <100.00%> (ø)
datalad/support/due.py 48.00% <0.00%> (-32.00%) ⬇️
datalad/ui/utils.py 46.87% <0.00%> (-25.00%) ⬇️
datalad/support/archive_utils_patool.py 29.87% <0.00%> (-10.39%) ⬇️
datalad/tests/test_misc.py 90.00% <0.00%> (-10.00%) ⬇️
datalad/metadata/tests/test_extract_metadata.py 91.11% <0.00%> (-8.89%) ⬇️
datalad/support/tests/test_path.py 92.50% <0.00%> (-5.00%) ⬇️
datalad/downloaders/http.py 81.85% <0.00%> (-4.82%) ⬇️
datalad/support/tests/test_due_utils.py 78.57% <0.00%> (-4.77%) ⬇️
... and 66 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 fef3c70...d243432. Read the comment docs.

@kyleam kyleam merged commit 3c4d99c into datalad:maint Sep 4, 2020
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-if-ok OP considers this work done, and requests review/merge UX user experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants