( )⚙ D1756 remotenames: introduce class to encapsulate remotenames info in an extension

This is an archive of the discontinued Mercurial Phabricator instance.

remotenames: introduce class to encapsulate remotenames info in an extension
ClosedPublic

Authored by pulkit on Dec 25 2017, 3:50 PM.

Details

Summary

This patch adds a new extension remotenames in which features from hgremotenames
extension (https://bb/seanfarley/hgremotenames) will be added incrementally.
This patch introduces a basic class to encapsulate the remotenames information.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

pulkit created this revision.Dec 25 2017, 3:50 PM

As per previous discussions, the storage part can only go to core directly (https://phab.mercurial-scm.org/D1358#22905). I am not sure about how out of core extension and in-core extension will interact. Also shall I change this extension name to prevent conflict with the out of core one?

In D1756#29970, @pulkit wrote:

As per previous discussions, the storage part can only go to core directly (https://phab.mercurial-scm.org/D1358#22905). I am not sure about how out of core extension and in-core extension will interact. Also shall I change this extension name to prevent conflict with the out of core one?

My 2¢: don't worry about the interactions. The existing remotenames is not published on pypi, so it's very unlikely people have it installed and active as just remotenames= in their extensions block.

No idea if others agree though.

In D1756#29970, @pulkit wrote:

As per previous discussions, the storage part can only go to core directly (https://phab.mercurial-scm.org/D1358#22905). I am not sure about how out of core extension and in-core extension will interact. Also shall I change this extension name to prevent conflict with the out of core one?

My 2¢: don't worry about the interactions. The existing remotenames is not published on pypi, so it's very unlikely people have it installed and active as just remotenames= in their extensions block.
No idea if others agree though.

In that case, I am happy to get this patches reviewed as they are.

durin42 accepted this revision.Jan 18 2018, 3:03 PM
This revision is now accepted and ready to land.Jan 18 2018, 3:03 PM
pulkit updated this revision to Diff 5145.Feb 2 2018, 2:47 AM
This revision was automatically updated to reflect the committed changes.
martinvonz added inline comments.
hgext/remotenames.py
23

Why does this extend dict? Do we expect callers to update bookmarks by doing something like the following? If not, and the extending of dict is just for the convenience of writing the loop in loadnames() below, we should do it differently and not expose the dict-ness to users. For example, this class could have a dict instead of being a dict.

remotenames = ...
remotenames['my-bookmark'] = node
smf added a subscriber: smf.Feb 20 2018, 5:45 PM

martinvonz (Martin von Zweigbergk) <phabricator@mercurial-scm.org> writes:

martinvonz added inline comments.
INLINE COMMENTS

remotenames.py:23
+
+class remotenames(dict):
+ """

Why does this extend dict? Do we expect callers to update bookmarks by doing something like the following? If not, and the extending of dict is just for the convenience of writing the loop in loadnames() below, we should do it differently and not expose the dict-ness to users. For example, this class could have a dict instead of being a dict.

remotenames = ...
remotenames['my-bookmark'] = node

When I wrote this, updating the bookmark (IIRC) was done through a
dict-like interface. Or perhaps I wanted it to be that way so that's how
I wrote it here.