libreoffice: Convert generate-libreoffice-srcs from Bash to Python #18755
Conversation
|
@chris-martin, thanks for your PR! By analyzing the annotation information on this pull request, we identified @7c6f434c to be a potential reviewer |
chris-martin
force-pushed
the
libreoffice-gen
branch
from
50cd7f6
to
5599213
Compare
| shellHook = '' | ||
| function generate { | ||
| ./generate-libreoffice-srcs.sh | tee libreoffice-srcs.nix | ||
| python3 generate-libreoffice-srcs.py > libreoffice-srcs.nix |
There was a problem hiding this comment.
Why not add #! /usr/bin/env python3 in the script, too? I think most NixPkgs developers have /usr/bin/env (it is default-on in NixOS)
Of course python3 here would stay anyway.
There was a problem hiding this comment.
I thought the lesson learned from NixOS was that shebangs were somewhat evil and /usr/bin/env was just a least-painful compromise. No problem adding it if you like, though.
There was a problem hiding this comment.
I agree - there's no harm in adding a shebang. I'd still leave the explicit call to python3 in this line though.
There was a problem hiding this comment.
This is what happens in the current version.
|
|
||
| def apply_additions(xs, additions): | ||
| for x in xs: | ||
| yield {**x, **additions.get(x['name'], {})} |
There was a problem hiding this comment.
I don't know, maybe if the problem you want to fix is low readability of the bash script to people with weak knowledge of bash, **x thing in Python could be worth a comment like «merge two hashtables into one» for people who don't write Python.
Oh, I see you have dict_merge, using it instead of **x would be self-documenting.
There was a problem hiding this comment.
Fair, will do.
| @@ -19,9 +19,11 @@ stdenv.mkDerivation { | |||
| builder = ./download-list-builder.sh; | |||
| }; | |||
|
|
|||
| buildInputs = [ python36 ]; | |||
There was a problem hiding this comment.
Why use a pre-release of Python 3.6? Please use python3 which at this time refers to python35.
There was a problem hiding this comment.
Ah, I didn't know 3.6 was considered pre-release. I think I just wanted string interpolation. Guess we can do without it.
Well, between no shebang and |
chris-martin
force-pushed
the
libreoffice-gen
branch
from
5599213
to
754836a
Compare
|
Added a shebang, changed package |
This is a rewrite of
generate-libreoffice-srcs.shin Python. The output of this script is identical to that of its predecessor.I think the result is easier to follow, and slightly less fragile. It does the variable substitution in a more general way, and relies less on the file's ordering.
It still prints skipped lines (and also line numbers) to stderr to help detect problems, though it doesn't print skipped comments.