This is an archive of the discontinued Mercurial Phabricator instance.

localrepo: deduplicate code around reading requires file
AbandonedPublic

Authored by pulkit on Jul 17 2020, 10:20 AM.

Details

Reviewers
marmoute
Group Reviewers
hg-reviewers

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

pulkit created this revision.Jul 17 2020, 10:20 AM
marmoute requested changes to this revision.Jul 20 2020, 1:11 PM
This revision now requires changes to proceed.Jul 20 2020, 1:11 PM
marmoute added inline comments.Jul 20 2020, 3:29 PM
mercurial/localrepo.py
535–549

This does not seems to need to be a closure. Can we make it a normal function?

marmoute resigned from this revision.Jul 20 2020, 3:44 PM

More serious question: Maybe we should stop passing around tuple of dictionnary and return an object holding all that data with a better API. What do you think ?

This revision now requires review to proceed.Jul 20 2020, 3:44 PM
marmoute requested changes to this revision.Jul 20 2020, 4:59 PM
marmoute added a subscriber: marmoute.

I resigned (and commented) on the wrong revision.

This revision now requires changes to proceed.Jul 20 2020, 4:59 PM
pulkit added inline comments.Jul 21 2020, 9:07 AM
mercurial/localrepo.py
535–549

This code ideally should not be used outside this function. Hence a closure.

marmoute added a comment.EditedJul 21 2020, 9:12 AM

That is a bad reason to use a closure. Make the function private and move it at the module level.
(Mercurial used to be full of this strange closure and this was a pain)

pulkit updated this revision to Diff 22000.Jul 21 2020, 9:42 AM
marmoute requested changes to this revision.Jul 21 2020, 9:46 AM
marmoute added inline comments.
mercurial/localrepo.py
535–549

Still a closure, please make it not a closure.

This revision now requires changes to proceed.Jul 21 2020, 9:46 AM
pulkit updated this revision to Diff 22069.Jul 23 2020, 10:52 AM
pulkit added inline comments.Jul 23 2020, 10:56 AM
mercurial/localrepo.py
535–549

Fixed.

pulkit updated this revision to Diff 22172.Aug 1 2020, 10:36 AM
pulkit abandoned this revision.Aug 7 2020, 2:51 PM