This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.
osmith gerrit-no-reply at lists.osmocom.orgosmith has posted comments on this change. ( https://gerrit.osmocom.org/11560 )
Change subject: replace src/* git scripts with a single src/gits
......................................................................
Patch Set 4: Code-Review-1
(19 comments)
https://gerrit.osmocom.org/#/c/11560/4/src/gits
File src/gits:
https://gerrit.osmocom.org/#/c/11560/4/src/gits@1
PS4, Line 1: #!/usr/bin/env python3
Add the copyright and SPDX license lines here?
https://gerrit.osmocom.org/#/c/11560/4/src/gits@2
PS4, Line 2: argparse
argparse is not used.
But I would highly recommend to use it:
* the help output would look like in any other python program
* right now "./gits -h" runs "git -h" in every directory instead of printing the help output
* no wheel-reinventing for checking the arguments, cmd_help(), having aliases
https://gerrit.osmocom.org/#/c/11560/4/src/gits@11
PS4, Line 11: re_status_mods = re.compile('^\t(modified|deleted):.*')
This is only used once in the code. There are other regex patterns, that are not global variables in this file. So I'd recommend to follow that pattern and move this to the function where it is needed.
https://gerrit.osmocom.org/#/c/11560/4/src/gits@29
PS4, Line 29: print(' '.join(cmd))
How about differentiating the command from the output it produces? For example with a leading '>' character:
> git -C osmo-mgw status
On branch master
Your branch is up-to-date with 'origin/master'.
nothing to commit, working dir clean
https://gerrit.osmocom.org/#/c/11560/4/src/gits@37
PS4, Line 37: sys.stderr.flush()
why flush the output only when the section_marker is needed?
https://gerrit.osmocom.org/#/c/11560/4/src/gits@45
PS4, Line 45: m
how about "branch" as variable name?
https://gerrit.osmocom.org/#/c/11560/4/src/gits@68
PS4, Line 68: up to date
With git 2.11.0 I get:
unknown status str: "Your branch is up-to-date with 'origin/master'."
(There's "git status --porcelain" which is meant to be parsed, but it doesn't print the branch line - maybe there's an appropriate git plumbing command with stable output that can be used instead?)
https://gerrit.osmocom.org/#/c/11560/4/src/gits@79
PS4, Line 79: m
How about "status"?
https://gerrit.osmocom.org/#/c/11560/4/src/gits@87
PS4, Line 87:
here are two empty lines after the function, mostly this file has one empty line instead. PEP8 would be using two empty lines between each function.
https://gerrit.osmocom.org/#/c/11560/4/src/gits@89
PS4, Line 89: '''return a list of strings: [branchname, info0, info1,...]'''
it's not clear what info0, info1 etc. would be. Maybe add examples with actual strings?
https://gerrit.osmocom.org/#/c/11560/4/src/gits@91
PS4, Line 91: interesting_branch_names = [ 'master' ]
:
: strs = [git_dir,]
(inconsistent: [ text ] vs [text,])
How about renaming "strs" to "ret" to indicate that this will be returned?
https://gerrit.osmocom.org/#/c/11560/4/src/gits@122
PS4, Line 122: strs.append(''.join(branch_info))
why make a list first, and then join it?
https://gerrit.osmocom.org/#/c/11560/4/src/gits@194
PS4, Line 194: print('\n===== %s =====' % git_dir)
If we print out something like "> cd %s && %s" before the output of the command, it would be consistent with executing git in every folder.
https://gerrit.osmocom.org/#/c/11560/4/src/gits@200
PS4, Line 200: SkipThisRepos
How about "SkipThisRepo"? repo is commonly used to abbreviate repository, so when reading "repos" I'm thinking this is a plural form.
https://gerrit.osmocom.org/#/c/11560/4/src/gits@207
PS4, Line 207: repos
"repo"?
https://gerrit.osmocom.org/#/c/11560/4/src/gits@223
PS4, Line 223: if v == answer:
: return answer
: if v == '*':
: return answer
how about:
if v in (answer, "*"):
return answer
(somehow gerrit puts this comment in a single line without writing something below the code block)
https://gerrit.osmocom.org/#/c/11560/4/src/gits@227
PS4, Line 227: if v == '+' and len(answer) > 0:
(the > 0 is not needed)
https://gerrit.osmocom.org/#/c/11560/4/src/gits@359
PS4, Line 359: cmd_do(sys.argv[1:])
I'm not sure if it's a good idea to perform "do" by default - whenever there's a typo in any of the actions, it will be executed in every directory.
https://gerrit.osmocom.org/#/c/11560/4/src/gits@362
PS4, Line 362: # vim: shiftwidth=2 expandtab tabstop=2
Since there are not Python coding guidelines in the wiki yet:
https://osmocom.org/projects/cellular-infrastructure/wiki/Coding_standards
How about following PEP8 (Style Guide for Python Code) instead?
https://www.python.org/dev/peps/pep-0008/
"Use 4 spaces per indentation level."
There are nice tools like "flake8" which automatically check the formatting, and also auto formatting tools like "autopep8".
--
To view, visit https://gerrit.osmocom.org/11560
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-dev
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I579e7af26d76d5c5d83b2349695456bc7b54f5a2
Gerrit-Change-Number: 11560
Gerrit-PatchSet: 4
Gerrit-Owner: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: osmith <osmith at sysmocom.de>
Gerrit-Comment-Date: Mon, 05 Nov 2018 11:31:27 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20181105/c642930a/attachment.htm>