Daniel Sahlberg wrote on Mon, Mar 08, 2021 at 09:15:21 +0100:
> Attaching version 5, with changes as below.
>
> 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
$
.
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?
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
