[PATCH] util: rename _windows_reserved_filenames and _windows_reserved_chars

Adrian Buehlmann adrian at cadifra.com
Sun May 8 05:29:26 CDT 2011


On 2011-05-08 11:25, Jason Harris wrote:
> 
> On May 8, 2011, at 10:03 AM, Adrian Buehlmann wrote:
> 
>> On 2011-05-08 09:50, Sune Foldager wrote:
>>> On 2011-05-08 09:41, Adrian Buehlmann wrote:
>>>> On 2011-05-08 08:57, Sune Foldager wrote:
>>>>> On 08-05-2011 00:29, Adrian Buehlmann wrote:
>>>>>> On 2011-05-07 23:38, Sune Foldager wrote:
>>>>>>> On 2011-05-07 23:07, Adrian Buehlmann wrote:
>>>>>>>> # HG changeset patch
>>>>>>>> # User Adrian Buehlmann<adrian at cadifra.com>
>>>>>>>> # Date 1304799920 -7200
>>>>>>>> # Node ID 4168febc29ffaee26de5cc1b1289454e4c85debf
>>>>>>>> # Parent  fe2153fd0fe5de49790e82d8038f5e07dd502e63
>>>>>>>> util: rename _windows_reserved_filenames and _windows_reserved_chars
>>>>>>>
>>>>>>> I realize we have this no-underscore policy, which I don't agree with, but is
>>>>>>> this really supposed to be an improvement?:
>>>>>>>
>>>>>>>> -_windows_reserved_filenames = '''con prn aux nul
>>>>>>>> +_winresnames = '''con prn aux nul
>>>>>>>
>>>>>>> Res? Resource names?... this is the primary reason I am opposed to
>>>>>>> no-underscores :p.
>>>>>>
>>>>>> Yeah. In *that* code, it is really hard see what _winresnames is
>>>>>> supposed to be hmm? You even had to delete the interesting bits, to
>>>>>> prove your point :-)
>>>>>
>>>>> Well, I didn't claim to *prove* anything. I just don't like shortening
>>>>> such names just so they can look less ridiculous when you remove the
>>>>> underscores from them.
>>>>>
>>>>>> Oh. Yeah. Let's see. And the function is called checkwinfilename. What
>>>>>> the heck is win again. Hmmm.
>>>>>
>>>>> Well, I think 'win' is a bit more established and unambiguous than 'res'.
>>>>>
>>>>>> I'm sure the next email is coming from Martin :-)
>>>>>
>>>>> Oh come on now... et tu?
>>>>
>>>> Instead of trolling and continuing to declare that you disagree, you
>>>> could propose a compromise or do something constructive.
>>>
>>> Easy now, tough guy. I think you're doing the trolling around here. I didn't
>>> take it personal or bing other people into it. Also, what I do or don't do is
>>> none of your business, so get off my back and mind YOUR own bussiness.
>>>
>>> Critique of patches is what we do here, so if you don't like it, don't post
>>> them.
>>
>> Arrogant and poisonous as usual (note this is a critique).
> 
>> The goal of this patch is to bring these names in line with the current
>> official project policy as stated at
>>
>> http://mercurial.selenic.com/wiki/CodingStyle
>>
>> Can you please explain how you want to have these names in this patch
>> changed, such they comply with that?
>>
>> Or what is the purpose of your critique?
> 
> As a person on the side lines... Reading the Mercurial source code is not
> as clear as it could be. The trend in most modern systems is to have self
> documenting code with long names. Eg Cocoa, Qt, Java, etc. having the
> munged names used in Mercurial makes the code very hard to read for
> casual coders / experimenters.

I feel quite the opposite. The Mercurial code is very readable. What's more,
if you have to ponder some difficult algorithms/data structures, these long
word chain names found in other programs are making a lot of optical noise and
distract my thinking.

I think a function call name like lets_do_x_and_then_do_z() popping up all
over a piece of code is very distracting. I have to read the whole word
chain each time to verify it's the same.

And please stop telling me you have a hard time to guess what _winresnames
could be in this context. I'll paste the full function below, so everyone
can see how ridiculously small it is (please allow me to omit the doctests
for that purpose):

_winresnames = '''con prn aux nul
    com1 com2 com3 com4 com5 com6 com7 com8 com9
    lpt1 lpt2 lpt3 lpt4 lpt5 lpt6 lpt7 lpt8 lpt9'''.split()
_winreschars = ':*?"<>|'
def checkwinfilename(path):
    '''Check that the base-relative path is a valid filename on Windows.
    Returns None if the path is ok, or a UI string describing the problem.
    '''
    for n in path.replace('\\', '/').split('/'):
        if not n:
            continue
        for c in n:
            if c in _winreschars:
                return _("filename contains '%s', which is reserved "
                         "on Windows") % c
            if ord(c) <= 31:
                return _("filename contains %r, which is invalid "
                         "on Windows") % c
        base = n.split('.')[0]
        if base and base.lower() in _winresnames:
            return _("filename contains '%s', which is reserved "
                     "on Windows") % base
        t = n[-1]
        if t in '. ':
            return _("filename ends with '%s', which is not allowed "
                     "on Windows") % t

So, from reading the doc string of that function, we get it is about
checking file names.

So how on earth can a reader's brain be distracted to suddenly start
guessing that "res" in "winresnames" could be "resource"?

Especially if you see:

        if base and base.lower() in _winresnames:
            return _("filename contains '%s', which is reserved "
                     "on Windows") % base

I think that snippet has a lot less optical noise and is much
easier to read than:

        if base and base.lower() in _windows_reserved_filenames:
            return _("filename contains '%s', which is reserved "
                     "on Windows") % base

However, this piece of function is not a particularly good argument for
having names like _winresnames, since in that function the name
_winresnames is used exactly once.

But if you have other snippets of code where such a name is used multiple
times, I find reading names like _winresnames much more comfortable than
reading _windows_reserved_filenames again and again all over that place.

Besides, Matt has already decided this (he prefers short lowercase names
without underbars).

Folks, can we please accept that and stop bickering about it?


More information about the Mercurial-devel mailing list