> On 9 Mar 2021, at 20:09, Daniel Shahaf <d...@daniel.shahaf.name> wrote:
>
> Daniel Sahlberg wrote on Mon, Mar 08, 2021 at 09:15:21 +0100:
>> Attaching version 5, with changes as below.
There are lines with trailing white space that should be cleaned up.
>>
>> Den sön 7 mars 2021 kl 20:42 skrev Daniel Shahaf <d...@daniel.shahaf.name>:
>>
>>> Daniel Sahlberg wrote on Sun, Mar 07, 2021 at 19:34:15 +0100:
>>>> + # Parse arguments
>>>> + import argparse
>>>> + parser = argparse.ArgumentParser(
>>> ⋮
>>>> + # The file name is the md5encoding of the realm
>>>> + import hashlib
>>>> + m = hashlib.md5()
>>>
>>> Hmm. Should all function-local imports be grouped at the top of the
>>> function? I don't remember the convention here.
>>>
>>
>> Is there a Subversion specific Style Guide for Python? I didn't find
>> anything in HACKING or trunk/doc.
>
> I don't think there's anything written down, but I don't think we
> invented anything either. Python scripts are typically written to be
> importable:
> .
> $ printf '%s\n' 'def foo():' ' return 42' > foo.py
> $ python3 -c 'import foo; print(foo.foo())'
> 42
> $
Give the file name it has it cannot be imported, this is a mute point.
The hypens would need to be underscores.
I see that many of the .py files in the folder cannot be imported either.
> .
> Thus, putting the imports in main() rather than at the top level means
> they don't get executed when they aren't needed.
>
>> PEP 8 says:
>> [[[
>> Imports are always put at the top of the file, just after any module
>> comments and docstrings, and before module globals and constants.
>> ]]]
>>
>> If I understand this correctly, it prefers global imports only? If keeping
>> the imports in the function, then moving to the top is probably more
>> PEP-8-ish. I didn't change anything though.
>
> In CPython's own source code,
> .
> rm -rf Lib/test && grep -Rn ' import' Lib Tools | perl -lnE '/(\d+)/;
> print if $1 > 100'
> .
> has over 400 matches. Spot checking a few (shuf(1) is my friend ☺),
> they seem like true positives.
>
>> For reference, ordering should be
>> [[[
>> Imports should be grouped in the following order:
>> 1. standard library imports
>> 2. related third party imports
>> 3. local application/library specific imports
>> ]]]
>> No explicit mentioning of alphabetical sorting, but that seems like a good
>> idea.
>>
>> (Offtopic: When reviewing /trunk/doc, I had a look at
>> /trunk/doc/programmer/WritingChangeLogs.txt. Is it still relevant? It seems
>> very CVS-ish and mostly covered in HACKING).
>
> Everything up to the last two section headers is good advice for writing
> log messages. For those parts, they should be added to HACKING or
> referenced therefrom.
>
> The remaining two sections don't seem applicable to us at all, but as
> the file is "an essay by jimb", I don't think we can change it.
>
>> +++ contrib/client-side/store-plaintext-password.py (working copy)
>> @@ -0,0 +1,189 @@
>> +def _read_one_datum(fd, letter):
> ⋮
>> + # Read the letter and the space
>> + if fd.read(1) != letter or fd.read(1) != b' ':
>> + raise ...
>
> (Raising ellipses is discussed elsethread.)
>
>> +def outputHash(fd, hash):
>> + """\
>> + Write a dictionary to an FD in the same format as used by
>
> Say the formal parameter's name somewhere in here?
Why have a back slash at all?
I avoid them in python code because of the problems of \<SP>.
>
> Calling it "hash" could be confusing, because svn_hash_write2() and
> hashlib use that term in different senses, but I don't have a suggestion
> about this.
>
>> +def writeHashFile(filename, hash):
>> + """\
>> + Open a temporary file and write the dictionary to the temp
>> + file in svn_hash_write2() format.
>> +
>> + If the file is successfully written it is renamed to <filename>.
>
> Please have all docstrings use the same markup for formal parameter
> names. outputHash() above uses the "capitalize the name" convention the
> C code uses for non-public-API function. I'll also use that for the
> example I write in the next paragraph.
>
> Describe the contract, not the implementation. For instance, "Write the
> dictionary HASH to a file named FILENAME in svn_hash_write2() format."
> would be a reasonable first sentence. The docstring could then promise
> a postcondition that if the file exists upon return, then it's not
> truncated, mention what happens if at entry FILENAME is a regular file
> or an unbroken symlink, etc..
>
>> + """
>> + tmpFilename = filename + '.tmp'
>> + try:
>> + with open(tmpFilename, 'xb') as fd:
>> + outputHash(fd, hash)
>> + os.rename(tmpFilename, filename)
>> + except FileExistsError:
>> + print('{}: File {!r} already exist. Is the script already
>> running?\n'.format(argv[0], tmpFilename), file=sys.stderr)
>
> s/the/this/
>
> Wrap to 80 columns.
>
>> + except:
>> + os.remove(tmpFilename)
>> + raise
>> +
>> +def main():
>> + # Parse arguments
>> + import argparse
>> + parser = argparse.ArgumentParser(
>> + description=PARSERDESCR,
>> + formatter_class=argparse.RawDescriptionHelpFormatter)
>> + parser.add_argument('realm', help='Server authentication real')
>> + parser.add_argument('-u', '--user', help='Set username', nargs='?')
>
> Just delete nargs entirely, for reasons given upthread? Sorry for
> missing that in my reviews between then and now.
>
>> + args = parser.parse_args()
>> +
>> + # The file name is the md5encoding of the realm
>> + import hashlib
>> + m = hashlib.md5()
>> + m.update(args.realm.encode('utf-8'))
>> + authfileName =
>> os.path.join(os.path.expanduser('~/.subversion/auth/svn.simple/'),
>> m.hexdigest())
>> +
>> + # If new file, check that username was given as an argument before
>> prompting for password
>> + existingFile = os.path.exists(authfileName)
>> + if not existingFile and args.user is None:
>> + print('{}: New file, username required (see -u)\n'.print(argv[0]),
>> file=sys.stderr)
>
> The phrase "new file" in the comment isn't clear, probably because it
> isn't a full sentence.
>
> The phrase "New file" in the error message has the same problem, as well
> as being phrased in implementation terms rather than in user-facing
> terms.
>
>> + parser.print_help()
>> + return
>
> The exit code should be non-zero whenever an error was printed to
> stderr.
>
> Aren't you reimplementing one of
> https://docs.python.org/3/library/argparse.html#exiting-methods
> ?
>
> Cheers,
>
> Daniel
>
Barry
