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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove superfluous AnnexRepo.remove() implementation #6783

Merged
merged 1 commit into from
Jul 2, 2022

Conversation

mih
Copy link
Member

@mih mih commented Jul 1, 2022

By moving the Repo.precommit() call into the actual
implementation in GitRepo. No change in API or functionality.

This scores a point in #4595

Changelog

馃彔 Internal

  • Consolidate GitRepo.remove() and AnnexRepo.remove() into a single implementation.

By moving the `Repo.precommit()` call into the actual
implementation in `GitRepo`. No change in API or funcationality.

This scores a point in datalad#4595
@mih mih added the semver-internal Changes only affect the internal API label Jul 1, 2022
@codeclimate
Copy link

codeclimate bot commented Jul 1, 2022

Code Climate has analyzed commit a36c33c and detected 0 issues on this pull request.

View more on Code Climate.

Copy link
Member

@bpoldrack bpoldrack left a comment

Choose a reason for hiding this comment

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

While I generally agree with having GitRepo handle this, you drop the force option from signature. It should still function (due to to_options), but now it's invisible to the caller. Maybe just make it explicit in GitRepo.remove?

Copy link
Member

@bpoldrack bpoldrack left a comment

Choose a reason for hiding this comment

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

To be clear: Fine with a merge, though, but mention it in changelog, if you go with unifying the other way around (no explicit force, but any git-rm option being avaiable via **kwargs period).

Copy link
Member

@yarikoptic yarikoptic left a comment

Choose a reason for hiding this comment

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

Makes total sense to me ! Checked other methods in AnnexRepo and it is only set_metadata_ which is calling .precommit, and no other ... In principle AFAIK, all parallel batched git-annex'es are operating with some awareness of each other due to journaling their operation and that is why things seems to remain working even though we do not have .precommit in e.g. _call_annex. So to some degree comment about "manifest ... on disk" is also incomplete/off. ATM it is indeed for changes to manifest in the tree and git index AFAIK.

@yarikoptic
Copy link
Member

CI run was done probably before chardet versioning workaround was introduced so travis is full of fails... I would assume they aren't related and appveyor is largely green, so let's just proceed

@yarikoptic yarikoptic merged commit 3303909 into datalad:master Jul 2, 2022
@bpoldrack bpoldrack mentioned this pull request Jul 7, 2022
40 tasks
@mih mih deleted the rf-remove branch July 19, 2022 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-internal Changes only affect the internal API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants