Closed toastal closed 6 months ago
Hold up… tiny tweak needed to delete a line.
check nix-format
Cute. The CI already shows it.
Thanks for this work! I will give it a proper review this week.
In the meantime — totally fine to upgrade to 23.11, there isn’t a specific reason to avoid upgrades. The VM uses 23.11 so we can test the same configuration that the server uses. This repository deploys on commits to master.
At a glance I’m in favor of the changes you’ve made. I’m but a mere Nix dabbler so most of this looks like optimizations I ought to have made from the start.
@thomashoneyman I ‘snuck’ in a commit about system.stateVersion moving to 23.11 as you mentioned. Tests passing locally.
@toastal Oh, I thought you meant using nixpkgs 23.11 — while I think it’s fine to change the state version (we’re not using anything that relies on it that I know of, like postgres) I don’t see the harm in leaving that alone. What’s the advantage of changing it?
What it will affect the most is the modules where settings can change if packages are updated & their configs don’t match. Keeping the packages & state in sync isn’t the worst idea. Sorry if I misunderstood what you meant. -- toastal ไข่ดาว | https://toast.al PGP: 7944 74b7 d236 dab9 c9ef e7f9 5cce 6f14 66d4 7c9e
@thomashoneyman hold on… I re-read & now I’m re-unclear. Do you want me to drop b2fa76d5a7cf5c6279afb480cce8378954ff47f3
(system.stateVersion: 23.05 → 23.11) or no?
It’s ok to upgrade that field since we’re not using anything that’s reliant on the state version. (Yet, at least.)
There are options to fix the situation, but it involves modifying .tidyrc.json or relaxing .editorconfig :| As it stands, the project isn’t compliant with its .editorconfig.
The source of truth is the .tidyrc so we should relax .editorconfig in that case. We don't use unicode symbols in the codebase.
cool
If Microsoft GitHub supported stacked diffs this would look less bad… this is meant to be read patch by patch, including the patch message (not just title) where it’s actually not too bad to read IMO.
Anyhow: I was looking to do a bit of hacking on this project, but the Nix build times & flake check timings were slow (CONTRIBUTING says to run
nix flake check
) & the Nix formatting was inconsistent unlike the PureScript.Highlights:
lib.fileset
to pick just the files needed to checknixfmt
-- the formatter specifieddevShell
(& looks like in the future will be defacto after it is tweaked)nixfmt
accross the project (& add it to the blame ignore)mkDerivation
which comes with a C compilerQuestions / follow-ups:
23.05
is the OS version for a VM & another system… should those upgrade or not? If no, I could pull in23.11
just forlib.fileset
& continue to use the older set for the packagessrc = ./.
as any time any file in the folder changes, the that derivation needs to rebuild, even when the changes don’t affect the files it touches (e.g. editing the project’s.purs
file shouldn’t make users rebuild the Spago & Node packages)); I don’t understand the project enough to guess how those are set up to know what the correct fileset would be, & the scope creep is enoughgit stash
a check foreditorconfig-checker
to verify.editorconfig
since there is a bug with howpurs-tidy
handles 2-char symbols for indentation that makes files no longer actually 2-space indented. While I love the ‘fix’ of turning Unicode on & ASCII off, it seems many don’t like it :(. There are options to fix the situation, but it involves modifying.tidyrc.json
or relaxing.editorconfig
:| As it stands, the project isn’t compliant with its.editorconfig
.Notes: