fira-code-symbols: 20160811 -> 20191205 #75034
Conversation
Vonfry
force-pushed
the
fix/fira-code-symbols-update
branch
from
c1702f2
to
e4b0f25
Compare
Generate font from current fira-code font instead of downloading the old one.
Vonfry
force-pushed
the
fix/fira-code-symbols-update
branch
from
e4b0f25
to
bffeb3c
Compare
ofborg
bot
added
11.by: package-maintainer
10.rebuild-darwin: 0
10.rebuild-linux: 1-10
labels
clean generate script, remove unused variable.
There was a problem hiding this comment.
Thanks for making your first contribution! This looks like a great first contribution. Most of the notes here are style/documentation-related.
A few other side notes:
- Make sure your commits & contributions fit CONTRIBUTING.md (specifically, squash the fira-code-symbols commits together)
- I'm not 100% sure the python script should be linked as source.
- Python environment looks a little funky, see comments.
| ''; | ||
| license = licenses.ofl; | ||
| maintainers = [ maintainers.Profpatsch ]; | ||
| maintainers = [ maintainers.Profpatsch maintainers.vonfry ]; |
There was a problem hiding this comment.
| maintainers = [ maintainers.Profpatsch maintainers.vonfry ]; | |
| maintainers = with maintainers; [ Profpatsch vonfry ]; |
| Use https://gist.github.com/xieve/d5a01cc59896c3973cb16df9ba8d30d4 script | ||
| to patch fira code with current version. |
There was a problem hiding this comment.
I'm not sure this needs to be in the description. I would rather have this information up in the source near where the script is actually called.
| mkdir -p $out/share/fonts | ||
| unzip -j $downloadedFile -d $out/share/fonts/opentype | ||
| ''; | ||
| phases = [ "buildPhase" "installPhase" ]; |
There was a problem hiding this comment.
It's atypical to override the phases, typically it's fine to let the phases do nothing. Is setting phases necessary?
| in stdenv.mkDerivation { | ||
| name = "fira-code-symbols-20191205"; | ||
| # https://gist.github.com/xieve/d5a01cc59896c3973cb16df9ba8d30d4 | ||
| src = ./fira_code_patch.py; |
There was a problem hiding this comment.
Do we need to have this script in-tree? Would it be better to grab it using something like fetchurl?
There was a problem hiding this comment.
Agreed. fetchurl using the gist commit is better.
| let pythonEnv = python3.withPackages (p: [ p.fontforge ]); | ||
| in stdenv.mkDerivation { | ||
| name = "fira-code-symbols-20191205"; | ||
| # https://gist.github.com/xieve/d5a01cc59896c3973cb16df9ba8d30d4 |
There was a problem hiding this comment.
Can you add a few words of detail to this gist link? What the script does, that the script is from this link, etc.
|
|
||
| fetchzip { | ||
| name = "fira-code-symbols-20160811"; | ||
| let pythonEnv = python3.withPackages (p: [ p.fontforge ]); |
There was a problem hiding this comment.
I honestly don't know whether this is best practice. This feels a little wrong to me to build a python environment for building an in-tree dependency...
Maybe something like this would work:
- add pythonFontForge as a dependency (set equal to python3Packages.fontforge in calling package)
- Add pythonFontForge to nativeBuildInputs
- check that the fontforge package is available in buildphase (something like
python -m fontforge)
Just guessing here.
|
|
||
| sha256 = "19krsp22rin74ix0i19v4bh1c965g18xkmz1n55h6n6qimisnbkm"; | ||
| buildPhase = '' | ||
| python3 $src -o . ${fira-code}/share/fonts/opentype/*.otf |
There was a problem hiding this comment.
no need to specify python3, I'd just do either python or ${python.interpreter}
| @@ -0,0 +1,50 @@ | |||
| #!/bin/env python3 | |||
There was a problem hiding this comment.
#! not necessary b/c it's being run by python interpreter explicitly
|
|
||
| import fontforge | ||
|
|
||
| def run(fontpath, outpath, starting_point=0xe100): |
There was a problem hiding this comment.
Docstrings somewhere in this code would be necessary to tell what it's doing.
| os.makedirs(output, exist_ok=True) | ||
|
|
||
| for f in chain.from_iterable(map(glob, fonts)): | ||
| run(f, os.path.join(output, os.path.basename(f))) |
There was a problem hiding this comment.
I personally prefer using python's pathlib library vs os, but if this script works, don't fix it!
|
I find this pr will break someone using the original font. Close this. |
Motivation for this change
The old version of fira-code-symbols is made from an old version fira-code. I use a script from the fira-code repo's issue to update it to current version of fira-code.
Things done
sandboxinnix.confon non-NixOS linux)nix-shell -p nix-review --run "nix-review wip"./result/bin/)nix path-info -Sbefore and after)Notify maintainers
cc @Profpatsch