Manually order config codecs #1106

thomashoneyman · 2023-10-23T18:56:54Z

Description of the change

Currently all JSON codecs use the Codec.Argonaut.Record module with its record sugar for defining objects. While convenient, this method will always order fields alphabetically in JSON, which isn't what we want in a config file. Instead we should manually order these fields. As an example, a fresh init of a config file will place the dependencies key before the package's name key; they really ought to go the other way around.

This PR switches to a manual encoding of the config formats.

Notes

I noticed that we generally reuse the YAML fields directly, such as exec_args being typed in PureScript as type ... = { exec_args :: ... }. If we want to be more purescript-y we can rename these fields in the codecs so the YAML is exec_args but the PureScript is execArgs.

The reason this particularly caught my attention is that we generally use snake case in YAML except for execArgs, which is camel case. Thought it may have been an oversight. But maybe it's intentional.

I'm also seeing some odd test failures but I'll report on them once the suite runs on this PR.

thomashoneyman · 2023-10-23T18:57:42Z

core/src/Config.purs

+configCodec = CA.object "Config"
+  $ CA.recordPropOptional (Proxy :: _ "package") packageConfigCodec
+  $ CA.recordPropOptional (Proxy :: _ "workspace") workspaceConfigCodec
+  $ CA.record


I generally stuck to the order I saw defined in the type synonyms, with the exception of potentially long lists (like extra_packages), which I generally moved to the end.

Let me know if you want anything ordered differently and it's easy enough to change.

f-f · 2023-10-23T19:06:36Z

The reason this particularly caught my attention is that we generally use snake case in YAML except for execArgs, which is camel case. Thought it may have been an oversight. But maybe it's intentional.

It's an oversight and should be fixed 😄

(Unless we'd rather switch everything to camelCase. I don't have a strong preference)

thomashoneyman · 2023-10-23T19:06:58Z

One test failure:

I had some trouble tracking down where in the code this is exactly happening, but for some reason the test section below is not respecting the codec ordering and is always ordering dependencies before main. This is a departure from the rest of the fixtures, which behave correctly.

  ✗ adds test dependencies to the config file when the test section does not exist:
  "package:\n  name: aaa\n  dependencies:\n    - console\n    - effect\n    - prelude\n  test:\n    dependencies:\n      - foreign\n    main: Test.Main\nworkspace:\n  package_set:\n    registry: 29.3.0\n  extra_packages: {}\n"
≠
"package:\n  name: aaa\n  dependencies:\n    - console\n    - effect\n    - prelude\n  test:\n    main: Test.Main\n    dependencies:\n      - foreign\nworkspace:\n  package_set:\n    registry: 29.3.0\n  extra_packages: {}\n"

f-f

Sooo pretty 🙂

This is great - now that we encode the fields manually I'm up for changing the records to be CamelCase if you'd prefer that as well

core/src/Config.purs

f-f · 2023-10-23T19:16:05Z

I had some trouble tracking down where in the code this is exactly happening

My bet is on Config.js - we manually manipulate the yaml there and maybe there's some automatic sorting going on (IIRC there's an option to disable it)

f-f · 2023-10-23T19:36:31Z

Hmm, Yaml.stringify has a sortMapEntries which is the option I'm thinking about, but it defaults to false and we are not currently overriding it

thomashoneyman · 2023-10-23T20:24:07Z

src/Spago/Config.js

@@ -12,7 +12,7 @@ export function addPackagesToConfigImpl(doc, isTest, newPkgs) {

  const deps = (() => {
    if (isTest) {
-      const test = getOrElse(pkg, "test", doc.createNode({ dependencies: [], main: "Test.Main" }));
+      const test = getOrElse(pkg, "test", doc.createNode({ main: "Test.Main", dependencies: [] }));


This was the culprit.

thomashoneyman · 2023-10-23T20:26:18Z

README.md

@@ -5,7 +5,7 @@
 [![build](https://github.com/purescript/spago/actions/workflows/build.yml/badge.svg)](https://github.com/purescript/spago/actions/workflows/build.yml)
 [![Maintainer: f-f](https://img.shields.io/badge/maintainer-f%2d-f-teal.svg)](http://github.com/f-f)

-*(IPA: /ˈspaɡo/)*
+_(IPA: /ˈspaɡo/)_


Sorry, I updated this file for the use of execArgs -> exec_args and, as I use Prettier, it was auto-formatted. If you want me to revert the changes to this file then I can (though I personally agree with the changes made).

Yeah it's fine, these changes are fairly harmless

thomashoneyman · 2023-10-23T20:28:20Z

I'm up for changing the records to be CamelCase if you'd prefer that as well

I don't care one way or another, just wanted to check. If we did, it'd look something like this:

type MyObject = { blahBlah :: String }

codec = Profunctor.dimap to from $ CA.Record.object "MyObject"
  { blah_blah: CA.string
  }
  where
  to = Record.rename (Proxy :: _ "blah_blah") (Proxy :: _ "blahBlah")
  from = Record.rename (Proxy :: _ "blahBlah") (Proxy :: _ "blah_blah")

f-f · 2023-10-23T20:35:46Z

If we did, it'd look something like this

Oh I see - I thought it would come for free with the manual encoding, but instead it seems a little tedious so let's keep things as they are

bin/src/Main.purs

bin/src/Flags.purs

core/src/Config.purs

f-f · 2023-10-23T20:44:16Z

README.md

@@ -5,7 +5,7 @@
 [![build](https://github.com/purescript/spago/actions/workflows/build.yml/badge.svg)](https://github.com/purescript/spago/actions/workflows/build.yml)
 [![Maintainer: f-f](https://img.shields.io/badge/maintainer-f%2d-f-teal.svg)](http://github.com/f-f)

-*(IPA: /ˈspaɡo/)*
+_(IPA: /ˈspaɡo/)_


Yeah it's fine, these changes are fairly harmless

thomashoneyman · 2023-10-23T21:13:10Z

Sorry about that! Renamed just the two now.

thomashoneyman · 2023-10-23T21:13:41Z

Not sure about the macOS failures — are these flaky?

spago » run
  ✓︎ works at all
Running test in /var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/spago-nodejs/7368613235362d427376674a41584638796e4d6861586e4252684f4a716550465a746f6c6b2f6b4d446441756771472b66733d
  ✗ can pass stdin to the application:

  test timed out after 90000ms
Running test in /var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/spago-nodejs/7368613235362d4b6d43784c6a36554c784d55486352483632527964766a384634537a504d4c7a304851424232674f5a30513d
  ✗ can pass arguments to the application:

  test timed out after 90000ms
Running test in /var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/spago-nodejs/7368613235362d384d30555832524948586a585539307857306a7659615166456c4e326b41676e73696c5145502b797167553d

f-f · 2023-10-23T21:48:28Z

They are flaky, we think they are failing for some reason related to #1048

thomashoneyman · 2023-10-23T21:53:54Z

Also now a Windows failure, this time I believe because this line is not expected in the fixture:

Refreshing the Registry Index...\n

Is there a way to control this, perhaps by running all tests in --offline mode?

thomashoneyman · 2023-10-23T21:54:25Z

I think this is done as far as I'm concerned, so feel free to merge away if you are OK ignoring the tests!

f-f · 2023-10-23T22:00:15Z

Is there a way to control this, perhaps by running all tests in --offline mode?

We need to test the internet-reaching parts too so that's not an option. I believe the issue we're seeing is related to the Windows filesystem being funky (we use the modification time on a file to note down the last time we pulled the registry, and I wouldn't be surprised if this was flaky on Windows) - should be fixed as part of #1083

Update codec ordering

861abb5

thomashoneyman commented Oct 23, 2023

View reviewed changes

f-f approved these changes Oct 23, 2023

View reviewed changes

core/src/Config.purs Outdated Show resolved Hide resolved

core/src/Config.purs Outdated Show resolved Hide resolved

thomashoneyman added 2 commits October 23, 2023 16:23

execArgs -> exec_args

704baee

Swap order of test config creation in Config.js

5917a75

thomashoneyman commented Oct 23, 2023

View reviewed changes

f-f reviewed Oct 23, 2023

View reviewed changes

reduce use of exec_args

df06e17

f-f approved these changes Oct 23, 2023

View reviewed changes

thomashoneyman merged commit 1694784 into master Oct 23, 2023
3 checks passed

thomashoneyman deleted the trh/codec-order branch October 23, 2023 22:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Manually order config codecs #1106

Manually order config codecs #1106

thomashoneyman commented Oct 23, 2023

thomashoneyman Oct 23, 2023 •

edited

Loading

f-f commented Oct 23, 2023

thomashoneyman commented Oct 23, 2023

f-f left a comment

f-f commented Oct 23, 2023

f-f commented Oct 23, 2023 •

edited

Loading

thomashoneyman Oct 23, 2023

thomashoneyman Oct 23, 2023

f-f Oct 23, 2023 •

edited

Loading

thomashoneyman commented Oct 23, 2023

f-f commented Oct 23, 2023

f-f Oct 23, 2023 •

edited

Loading

thomashoneyman commented Oct 23, 2023

thomashoneyman commented Oct 23, 2023

f-f commented Oct 23, 2023 •

edited

Loading

thomashoneyman commented Oct 23, 2023

thomashoneyman commented Oct 23, 2023

f-f commented Oct 23, 2023 •

edited

Loading

Manually order config codecs #1106

Manually order config codecs #1106

Conversation

thomashoneyman commented Oct 23, 2023

Description of the change

Notes

thomashoneyman Oct 23, 2023 • edited Loading

Choose a reason for hiding this comment

f-f commented Oct 23, 2023

thomashoneyman commented Oct 23, 2023

f-f left a comment

Choose a reason for hiding this comment

f-f commented Oct 23, 2023

f-f commented Oct 23, 2023 • edited Loading

thomashoneyman Oct 23, 2023

Choose a reason for hiding this comment

thomashoneyman Oct 23, 2023

Choose a reason for hiding this comment

f-f Oct 23, 2023 • edited Loading

Choose a reason for hiding this comment

thomashoneyman commented Oct 23, 2023

f-f commented Oct 23, 2023

f-f Oct 23, 2023 • edited Loading

Choose a reason for hiding this comment

thomashoneyman commented Oct 23, 2023

thomashoneyman commented Oct 23, 2023

f-f commented Oct 23, 2023 • edited Loading

thomashoneyman commented Oct 23, 2023

thomashoneyman commented Oct 23, 2023

f-f commented Oct 23, 2023 • edited Loading

thomashoneyman Oct 23, 2023 •

edited

Loading

f-f commented Oct 23, 2023 •

edited

Loading

f-f Oct 23, 2023 •

edited

Loading

f-f Oct 23, 2023 •

edited

Loading

f-f commented Oct 23, 2023 •

edited

Loading

f-f commented Oct 23, 2023 •

edited

Loading