aspellDicts: add more dictionaries and some documentation#34798
aspellDicts: add more dictionaries and some documentation#347987c6f434c merged 1 commit intoNixOS:masterfrom
Conversation
d04939a to
7c79305
Compare
|
this redirects
Fixed, I hope.
|
|
@GrahamcOfBorg eval |
There was a problem hiding this comment.
In both cases you set phases, why? Should this be improved in buildDict?
9d0f039 to
10a3068
Compare
|
In both cases you set `phases`, why? Should this be improved in `buildDict`?
s/you/the author/
I wondered myself, but now it seems that it is by design. `preBuild`
simply defines a bunch of functions, `buildPhase`s and `installPhases`s
are different for different derivations, other default phases would
either fail or are useless.
But its true I can just move it to `buildDict`. Done. Also made another
trivial change.
|
There was a problem hiding this comment.
We should not be using propagatedUserEnvPackages
There was a problem hiding this comment.
This only considers the method where dicts are installed separate from aspell, and not aspellWithDicts. Perhaps there is a better place for this comment? Otherwise, it's good to have the explanation!
There was a problem hiding this comment.
Why are these needed? Should it not detect them?
There was a problem hiding this comment.
Quotes are not needed, and until the policy changes, they should not be included either.
There was a problem hiding this comment.
That's super annoying when upgrading a literal URL to a ${version} or ${name} one. Has this policy finally been written down somewhere?
There was a problem hiding this comment.
How about creating instead a src that can be consumed by the old buildDict? Any reason for not doing that?
There was a problem hiding this comment.
Any motivation for changing the name?
10a3068 to
fbb9d9b
Compare
|
We should not be using `propagatedUserEnvPackages`
Any suggestions with what to replace it with? You should have `en_US` in the
profile when installing `en_US-science`.
and aspell itself?
Fixed.
This only considers the method where dicts are installed separate from aspell, and not `aspellWithDicts`. Perhaps there is a better place for this comment? Otherwise, it's good to have the explanation!
The problem with `aspellWithDicts` is that it does `--set ASPELL_CONF` which breaks the above explanation.
You would need to implement `aspellWithDictsAndConfig` to make this work. This is outside of scope of this PR.
Why are these needed? Should it not detect them?
They are "extra", opt-in.
Quotes are not needed, and until the policy changes, they should not be included either.
Official source on that claim? I'm motivated by #27809.
How about creating instead a `src` that can be consumed by the old `buildDict? Any reason for not doing that?
Pack files into an archive just to unpack them at the next beta-reduction? Seems crazy to me.
Any motivation for changing the name?
Right. Fixed.
|
fe141f7 to
0f8fe1a
Compare
|
Well whatever, the last patch has gone to SLNOS. To make my position clear: I see no policy behind #27809. What I see is somebody making a huge change to nixpkgs that goes backwards and you merging it without discussion. |
The closest would be
See #34856.
Agreed. |
|
The closest would be `aspellWithDicts`, for installing individual packages there is nothing else.
Right. But if I removed the propagation now it would break plain `systemPackages` installs.
Unless you put a check into `systemPackages` that rejects dictionaries I think the current impl is the correct one.
> The problem with `aspellWithDicts` is that it does `--set ASPELL_CONF` which breaks the above explanation.
See #34856.
Looks ok to me, needs extensive testing.
I propose we merge this first as it is well-tested (a whole year of fermentation in SLNOS) and makes things strictly better.
You can always change the docstrings later when `aspellWithDicts` becomes awesome.
|
0f8fe1a to
3f9ada0
Compare
|
OK, looks like this version is pure addition. |
Motivation for this change
Jargon files.
Things done
build-use-sandboxinnix.confon non-NixOS)nix-shell -p nox --run "nox-review wip"