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 deprecated (promoted to .core.local.run) in 0.12 release interface.run and saver option #4583

Merged
merged 2 commits into from Jun 23, 2020

Conversation

yarikoptic
Copy link
Member

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.

I don't think these changes should be made in the rc stage. And in general there is no reason this needs to be removed right now.

@yarikoptic
Copy link
Member Author

agree in general (thus did not label it for 0.13.0 milestone although said "likely" in containers PR ;) ). Will mark for 0.14.0.

And in general there is no reason this needs to be removed right now.

My selfish rationale for removal of those deprecated interfaces - pycharm allows to quickly open an file by starting to type its name (like run). Having multiple run, diff, etc across code base complicates navigation etc. Since I think we all would agree that they should be removed, I did not want to delay submitting a PR since I would have just forgotten and it would lurk around for longer.

@yarikoptic yarikoptic added this to the 0.14.0 milestone May 28, 2020
@codecov
Copy link

codecov bot commented May 28, 2020

Codecov Report

Merging #4583 into master will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4583   +/-   ##
=======================================
  Coverage   89.63%   89.63%           
=======================================
  Files         285      284    -1     
  Lines       38685    38677    -8     
=======================================
- Hits        34677    34670    -7     
+ Misses       4008     4007    -1     
Impacted Files Coverage Δ
datalad/core/local/run.py 99.13% <ø> (+0.42%) ⬆️

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 e7e42a4...ec4afad. Read the comment docs.

@yarikoptic
Copy link
Member Author

Logistics wise: should we establish next or 0.14 branch to which we merge such changes? Otherwise we would need to keep such PRs open until we decide that master is seeing no new features and destined for the new "major" release. IMHO version based (0.14) is better but I am ok with next.

@kyleam
Copy link
Contributor

kyleam commented Jun 10, 2020

Logistics wise: should we establish next or 0.14 branch to which we merge such changes?

If you think that is useful, that's fine by me. My opinion is that creating another branch to merge in changes like this adds no value and that merging this can wait until a release. The code being removed by this PR is not a maintenance burden.

FWIW, aside from stylistically disliking branch names that consist only of versions, I'd find 0.14 confusing as a name because I'm used to branches with versions being a track for a lesser version tick (e.g., the last tag in maint-2.23 could be, say, v2.23.3).

@mih
Copy link
Member

mih commented Jun 10, 2020

I do not see a need for another branch. Admittedly, I already had a hard time adjusting to maint. I am also not developing something for the release after the coming one. And I am also used to seeing lots of open PRs. It would become an issue for me, when there are 6 months of inactivity of a perfectly mergable PR, because I would close it as stale ;-)

@kyleam
Copy link
Contributor

kyleam commented Jun 10, 2020

It would become an issue for me, when there are 6 months of inactivity of a perfectly mergable PR, because I would close it as stale ;-)

I think there are deeper issues to worry about if 0.13.0 stays in the rc phase for 6 months.

@kyleam kyleam merged commit a655d9f into datalad:master Jun 23, 2020
@yarikoptic yarikoptic deleted the rm-deprecated branch October 7, 2020 15:41
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

3 participants