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

RM: install - remove --nosave/save=False #5278

Merged
merged 1 commit into from Jan 6, 2021

Conversation

yarikoptic
Copy link
Member

As it is of no effect for awhile -- I have not done investigation when it
started to be of no effect, but it was reported on the matrix forum that
it has no effect in 0.13.5-0.13.7

Since 0.14.0 seems to be not "revolutionary" enough , and option is of no effect, I think it would be ok to just kill it

As it is of no effect for awhile -- I have not done investigation when it
started to be of no effect, but it was reported on the matrix forum that
it has no effect in 0.13.5-0.13.7
@yarikoptic yarikoptic added the merge-if-ok OP considers this work done, and requests review/merge label Jan 5, 2021
@yarikoptic yarikoptic added this to the 0.14.0 milestone Jan 5, 2021
Copy link
Contributor

@kyleam kyleam left a comment

Choose a reason for hiding this comment

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

As it is of no effect for awhile -- I have not done investigation when it
started to be of no effect

Perhaps 21f64ea (RF+BK: Non-functional install RF, documents general approach, 2017-04-23)?

Anyway, sounds good to me. Thanks.

@kyleam
Copy link
Contributor

kyleam commented Jan 5, 2021

Anyway, sounds good to me.

Though perhaps someone has scripts that call install with save=True or save=False, and they didn't notice it had no effect. Rather than crashing their scripts, it might be nice to use a dummy parameter along the lines of _NoAnnexDefault and issue a warning saying the parameter has been a noop for a long time and will be removed in the next release.

@yarikoptic
Copy link
Member Author

Anyway, sounds good to me.

Though perhaps someone has scripts that call install with save=True or save=False, and they didn't notice it had no effect. Rather than crashing their scripts, it might be nice to use a dummy parameter along the lines of _NoAnnexDefault and issue a warning saying the parameter has been a noop for a long time and will be removed in the next release.

what kind of a revolution would that be? ;) I would better wait for corpses in the basement (#3496, and #3496 (comment)) to come alive, i.e. not going to bother for this unlikely to be heavily used "feature". Feel welcome to take over

@kyleam
Copy link
Contributor

kyleam commented Jan 6, 2021

not going to bother for this unlikely to be heavily used "feature"

It's not about it being a feature. It's about avoiding potential script breakage when we have an easy way to.

But sure, I agree that perhaps nobody has every done install(..., save=True) without thinking, and I'm fine with your judgment call that this isn't worth worrying about.

@yarikoptic yarikoptic merged commit b6ed9fa into datalad:master Jan 6, 2021
@yarikoptic yarikoptic deleted the rm-install-save branch April 29, 2021 21:59
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants