spago
spago copied to clipboard
purescript
🎅 🐫 - Switch to camel case in config
Heh, CI fails because Spago can't build itself anymore - I think we should accept both formats for now, and keep the config in the old format until we have a new version out
I think it's a bit too much for the codebase to support two versions of the config. Keeping things backward compatible for alpha status software is not important imo. Isn't there another way too bootstrap the CI build once?
We have already bootstrapped in the past so that's not the issue - we are indeed in alpha, but at this point there are quite some packages using this version of spago, and this breaks 100% of them. We can save some of that inconvenience by supporting the old config as well for a while, e.g. until beta. This is not wastes effort because we'll eventually have to be able to support multiple versions of the config, once things get more stable.
Maybe if it can be either one or the other version of the config. I don't see it working out with mixed versions (differences per property field).
About breaking packages, they can update their yaml file to the new format and it should be good to go no?
I don't see it working out with mixed versions (differences per property field).
We wouldn't mix versions per field, we'd either accept the current config or the new one, no mix.
About breaking packages, they can update their yaml file to the new format and it should be good to go no?
The error that spago is throwing is not telling folks that they should upgrade to the new format. Adding such an error message would need detecting the old format, at which point we might as well just accept it.
Is there a yaml printer available? We could parse the old format and do the upgrade for them
Automatically upgrading the config format is a good idea, but it might take quite a few lines - we have a "yaml printer", but editing the configuration is not just a matter of "write the new content to replace the old content": once we parse the yaml into a PureScript type, we lose all the metadata that was contained into the original file, e.g. comments.
So we need to read the yaml file and keep around the rich representation that contains all this metadata, and whenever we want to edit the configuration we need to reflect back the edits into this format.
As an example, we do this for spago install some-package, here:
https://github.com/purescript/spago/blob/b450892d69aa3013861fde6c73299d2c17aba3dc/src/Spago/Config.purs#L507-L510
The above calls a piece of FFI that traverses the config in the format that the JS yaml library uses, and adds the right things in the right place.
If we want to automatically upgrade people's configs (which I think it's desirable) we would use a similar approach.
The above calls a piece of FFI that traverses the config in the format that the JS yaml library uses, and adds the right things in the right place. If we want to automatically upgrade people's configs (which I think it's desirable) we would use a similar approach.
Is that the last thing blocking this PR (aside from resolving any merge conflicts with master)?
We need this auto-upgrade the yaml file to camel case and then it's good to go
Are you going to work on that? Or do you need help with knowing how to do that?
I intend to but since I'm no longer on Christmas break it will be slower now to get to it. Feel free to take over if you have motivation, otherwise i will get to it hopefully rather sooner than later
Yes I think that once we recognise the old config and auto-upgrade it then we don't need to do much else here - we'll want to add a few tests to ensure that we don't regress on parsing old configs and are always able to port them.
I came up with a solution for auto-migrating the case situation.
The below JS code (which allowed for faster iteration in a dynamic context) works using a top-down approach by:
- getting the old value at a given path
- deleting that value in the doc
- adding it back in under a new name
Due to mutating the value, there's one difficulty with this approach. Let's specifically rename package and package.name by uppercasing them. When we hit the package key, we make the following change:
-package:
- name: foo
- description: bar
+PACKAGE:
+ name: foo
+ description: bar
What was previously located at package.name is now located at PACKAGE.name. So, we need to account for this in Step 1 above when getting the original value. Thus, I am currently storing the parent path and the final key part separately so that I can easily map between the two. The below script illustrates this.
getIn/deleteIn/addIn use an array of paths into the document to update that part of the document. getIn is slightly different in that one needs to additionally pass in true, so that it doesn't unwrap any scalar values.
Script
import Yaml from "yaml";
const doc = Yaml.parseDocument(`
package:
name: "foo"
bar: 1
workspace:
something: 1
`);
const paths =
[ { parent: [], name: "package" }
, { parent: ["package"], name: "name" }
];
const migrated = doc.clone();
const renameFn = (x) => x.toUpperCase();
paths.forEach(path => {
const oldFullPath = (() => {
const p = path.parent.slice().map(renameFn);
p.push(path.name);
return p;
})();
const newFullPath = (() => {
const p = path.parent.slice();
p.push(path.name);
return p.map(renameFn);
})();
if (migrated.hasIn(oldFullPath)) {
console.log(`\nFound a value at path '${oldFullPath.join(".")}'`);
const value = migrated.getIn(oldFullPath, true);
console.log(`Got value`);
console.log(value);
console.log("Deleting old value");
migrated.deleteIn(oldFullPath);
console.log("Readding original value");
migrated.addIn(newFullPath, value);
console.log("Doc is now...");
console.log(migrated.toJSON());
} else {
console.log(`\nNo value found at path ${path.join(".")}`);
}
});
console.log("\nFinal doc");
console.log(migrated.toJSON());
Output:
Found a value at path 'package'
Got value
YAMLMap {
items: [
Pair { key: [Scalar], value: [Scalar] },
Pair { key: [Scalar], value: [Scalar] }
],
range: [ 14, 37, 37 ]
}
Deleting old value
Readding original value
Doc is now...
{ workspace: { something: 1 }, PACKAGE: { name: 'foo', bar: 1 } }
Found a value at path 'PACKAGE.name'
Got value
Scalar {
value: 'foo',
range: [ 20, 25, 26 ],
source: 'foo',
type: 'QUOTE_DOUBLE'
}
Deleting old value
Readding original value
Doc is now...
{ workspace: { something: 1 }, PACKAGE: { bar: 1, NAME: 'foo' } }
Final doc
{ workspace: { something: 1 }, PACKAGE: { bar: 1, NAME: 'foo' } }
Not sure how this should deal with Arrays yet, but we can get the config's keys using type-level programming to produce an array of paths
I had a look at #1168 and had a chat with Jordan about the patch - we are both not very happy with the typeclass-based solution - it's very generic but quite noisy and fiddly.
I think a more lightweight approach would be to have a configV1Codec :: JsonCodec ConfigV1 that we use to read the old config, together with a foreign migrateV1Config :: YamlDoc ConfigV1 -> YamlDoc Config function (from Config.js) that just does the low-level manipulation of the config.
This is a pattern that we can apply for all the config migrations from here on and, while it's not as guaranteed to be correct for all cases, it's quite simple to handle.