Constant naming convention

Martin von Zweigbergk martinvonz at google.com
Tue Jan 10 12:29:25 EST 2017


On Tue, Jan 10, 2017 at 7:42 AM, Pierre-Yves David
<pierre-yves.david at ens-lyon.org> wrote:
>
>
> On 01/02/2017 11:27 PM, Kevin Bullock wrote:
>>
>> A few overall notes on this thread, and then some specific clarifications
>> below:
>>
>> First, it is not the current project policy to defer any decisions until
>> October 2017. It is also true that none of the core team is advocating
>> radical changes to the way we develop Mercurial, either before or after that
>> date.
>>
>> Second, this topic is a matter of project policy, so we should take it up
>> as the steering committee if we want to take any further action on it.
>
>
> To me, that part, and a large share of Ryan emails, highlights the fact that
> we need to spend more time at the steering committee level to define our
> decision process on multiple aspect of the project. For now, I would rather
> see energy put in these definition first, before we start discussing more
> practical aspect of the project as the one here.
> My general feeling is that we have been putting the card before the hoax
> here.
>
>>> On Dec 28, 2016, at 10:17, Pierre-Yves David
>>> <pierre-yves.david at ens-lyon.org> wrote:
>>>
>>> On 12/15/2016 11:38 PM, Augie Fackler wrote:
>>>>
>>>> (I’m trying to be brief here - hopefully it doesn’t come across as
>>>> upset, because I’m not - mostly I was blindsided by a policy claim that I
>>>> don’t remember.)
>>>>
>>>>> On Dec 15, 2016, at 3:40 PM, Pierre-Yves David
>>>>> <pierre-yves.david at ens-lyon.org> wrote:
>>>>>
>>>>> So, to sum up, my stance is "Let us not revisit decision made by Matt
>>>>> for a while". Of course they might be case were we could make an exception
>>>>> if it is really worth it. Code style does not seems to be high profile
>>>>> enough for that (and is a lot of work to adjust).
>>>>
>>>>
>>>> In broad strokes, I agree that we should make an effort to not cause a
>>>> wave of sea change. My perception of this particular issue is that:
>>>>
>>>> 1) The codebase is already somewhat inconsistent (despite the efforts of
>>>> mpm and others) when it comes to constant naming
>>>
>>>
>>> My perception (and apparently Kevin's too) is that the codebase is
>>> currently consistent¹. A very quick check of the current code seems to
>>> confirm it is consistent (but that was a quick check). So, I think you need
>>> to back this statement with stronger fact than just your perception.
>>
>>
>> No, I said there is a _prevailing_ style, not that we have consistency.
>> Augie was not the only one to draw the conclusion that we're not consistent
>> today. There are sufficient exceptions to question the strength of the rule.
>> Thank you for digging up the applicable section in CodingStyle, though. It
>> seems the convention is written down after all.
>
>
> There seems to be wording confusion here. So lets try to lift some of it.
> Over the course of this thread, I said the described the code base as:
>
>>> consistent for the very vast majority.
>
>
> and
>
>>> currently consistent¹
>>> […]
>>> [1] sure there is a handful of exception.
>
>
> This seemed compatible with your statement that we had a:
>
>> prevailing (though by no means universal) style
>
>
> You seems to disagree with that compatibility, sorry for misinterpreting
> you.
>
>
> That said, the question is still open of how consistent the current
> code-base is. In a previous email, I've asked for the people suggesting the
> changes to provide data to justify it. But so far I've only seen two peoples
> expressing feeling that the code base was too inconsistent. Feelings are
> great to bootstrap questions and start leads to potential improvements.
> However I don't think it is reasonable to take actions without gathering
> proper data first. So far, I've not seen actual data. Only well known
> anecdotal exception that don't provide serious information in themselves.
>
> So, I went to gather basic data about the actual situation. I used basic
> grepping of assignment without any indentation in mercurial/ and hgext/
> pythons file. This isn't perfect but that gives some basic data. I've
> filtered out vendor-ed code, many of the re-assignment from other modules
> and code directly tied to a specific OS API. Assignment of both type can
> contains '_', (especially leading '_'). Here are my finding:

For the record, if you include the vendored code (mostly
mercurial/win32.py, mercurial/httpclient/, hgext/fsmonitor/,
hgext/zeroconf/), you get this:

$ find mercurial/ hgext -name '*.py' | xargs grep '^[_A-Z0-9]\+ = ' |
grep -v ' = .*\..*' | wc -l
136

$ find mercurial/ hgext -name '*.py' | xargs grep '^[_a-z0-9]\+ = ' |
grep -v ' = .*\..*' | wc -l
436

I think it's fair to include this data so readers can decide for
themselves whether to include it or not. It's part of our code base,
after all.

>
> upper case assignment:
> ----------------------
>
> Only 4 modules or packages use "many" upper case constants. 3 is core, 1 in
> hgext (various parts of 'convert'). Adding the one with 1 or two instances,
> we got up to 10 modules/package with upper case constants, (about 40
> assignments total). Most of it is very old code (probably prior Matt started
> enforcing current style) and a few are recent slip in style-review.
>
> Detailed data below:
>
>   mercurial/revlog.py (10 cases)
>   mercurial/hgweb/ (9 cases)
>   mercurial/httpconnection.py 1 case
>   mercurial/util.py 2 case
>   mercurial/graphmod.py (5 spots)
>   hgext/factotum.py 1 cases
>   hgext/largefiles/ 1 cases
>   hgext/highlight/ 1 cases
>   hgext/mq.py 1 case
>   hgext/convert/ (8 case)
>
> Lower case assignment:
> ----------------------
>
> Full check on lower case assignments is harder because there is a large
> quantity of them. I've filtered out thing that standed out as vendored code
> and the vast majority of local assignments from other modules (eg 'pickle =
> util.pickle'). The result seems mostly composed of actual constant
> assignment for scanning through it.
>
> That filtering leaves about 500 instances of lower case assignments, spread
> over 120 files.
>
> Conclusion
> ----------
>
> Lets drop about 20% more of the lowercase assignments and files to take in
> account the imperfect filtering. This still lives use with a 1 vs 10 ration
> in favor of lower case for both "assignment count" and "module". This is a
> bit higher than what I expected but still show "upper case" as rare
> occurrence (<10%, handful of modules).
>
> So from my point of view, the occurrence is rare-enough and limited to
> few-enough module to consider the currently documented style (lower for all)
> as vastly prevailing and the code base as overall consistent.
> (I'm of course okay with hearing other conclusion based on these data (or
> better data)).
>
> It is also interesting to note that use of upper case is concentrated in
> some key module (eg: revlog, hgweb). That might play a key part in the
> strong contrast in consistency perception between the feeling of the two
> people who said the code to be too inconsistent and my feeling that is was
> overall consistent.
>
>
>
> Of course, The extraction I did is fairly basic. For example it does not
> takes in accounts indented, but still module level constants. In addition,
> the data about lower case assignment can be refined further. But I already
> spent more time digging this that I was willing to, so I did not went
> further. If someone else wants to keeps digging, feels free to do it.
>
>> […]
>
>
> Cheers,
>
> --
> Pierre-Yves David
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel


More information about the Mercurial-devel mailing list