Migrate to Camel Case (Discussion) #1168
Conversation
| migrationStep :: forall h a. ST h (YamlDoc a) -> YamlMigrationStep -> ST h (YamlDoc a) | ||
| migrationStep stDoc (YamlMigrationStep { key, toPrevKey, updateValue}) = do | ||
| doc <- stDoc | ||
| let oldKey = maybe key (\f -> f key) toPrevKey | ||
| when (has doc oldKey) do | ||
| value <- get doc oldKey true | ||
| for_ (toSeqItems value) \item -> | ||
| void $ migrationSteps (pure item) updateValue | ||
| when (isMap value) do | ||
| void $ migrationSteps (pure value) updateValue | ||
| for_ toPrevKey \_ -> do | ||
| delete doc oldKey | ||
| add doc key value | ||
| pure doc |
There was a problem hiding this comment.
First, starting with this code, this is the general idea we want to employ. Originally, I was using getIn and friends before I realized that just using get is simpler as it avoids the issue of a parent key being renamed before we mutate a value.
For sequence values, I'm iterating through each one automatically via the toSeqItems function. This uses a runtime check to determine whether or not the value cannot be modified using our function; rather, we need to modify each element in that sequence.
A similar check is done via isMap: we can recurse on the value if it's a Map but don't if it's a Scalar.
Lastly, we rename the key by deleting the old one and adding the new one. Since this is using type-level programming, we provide a function that maps the new key name to the old one. I've identified one problem with this but more on this later.
There was a problem hiding this comment.
Second, the gist of this file is that we require every type ever mentioned in Config to have an instance of MigrateFromSnakeCase'. This ensures that we are migrating every possible key that has a correspondence to a config field (e.g. package_set) without modifying keys that are themselves values to some config value (e.g. prelude as a dependency).
The advantage of this approach is that we have a guarantee that every type was checked (at least upon initial write). The disadvantage is that some instances (e.g. WarningCensorTest) require referencing a row-based type to ensure the changes stay synced. We could use a hard-coded value but there's no guarantee that that will stay in sync. In other cases involving types from the Registry, I can't use the WarningCensorTest workaround. So, that means implementing instances that will likely go out-of-sync if the Registry types change their internal Yaml/JSON decoding.
Another disadvantage is having to define and implement an entire type classes for every migration we want to support.
Regardless, here's an awkwardness that arises. Due to the camel case being just one possible migration, I've defined the FromSnakeCase class in its own module because its verbose and otherwise would distract from the code in core/src/Config.purs.
However, this same migration needs to (should?) be done in the Lock file. Ideally, we would reuse the same type class defined in core/src/Config/Migrations/FromSnakeCase.purs. However, I don't see a reason for why registry-dev should implement instances for FromSnakeCase because this is a Spago concern, not a Registry one. But by not doing that, we can't reuse this type class. So, we have to define a new one just for the Lock type, reimplementing the same instance for a given type (e.g. SetAddress) that provides another way for things to go out-of-sync.
| -- Values under this field should not be modified | ||
| instance | ||
| ( MigrateLockFromSnakeCase' v | ||
| ) => MigrateLockFromSnakeCase' (Map k v) where | ||
| migrateLockFromSnakeCase' _ = List.singleton $ | ||
| YamlMigrationStep | ||
| { key: "foo" | ||
| , toPrevKey: Nothing | ||
| , updateValue: migrateLockFromSnakeCase' (Proxy :: Proxy v) | ||
| } |
There was a problem hiding this comment.
Third, with the current design, this instance is incorrect as made obvious via .workspace.packages.<package>.test_dependencies. Similar to what I do with the sequence values, I'd need to modify YamlMigrationStep to specify whether the next value has a known key or whether one should just iterate through all of its keys.
| -- License is (at the time of writing) a newtyped String | ||
| instance MigrateFromSnakeCase' License where | ||
| migrateFromSnakeCase' _ = Nil | ||
|
|
||
| -- PackageName is (at the time of writing) a newtyped String | ||
| instance MigrateFromSnakeCase' PackageName where | ||
| migrateFromSnakeCase' _ = Nil |
There was a problem hiding this comment.
Fourth, these instances will go out-of-sync whenever these types change. And because they're from registry-dev, we'll encounter a similar issue as we did previously when spago-core need to be updated, then registry-dev, and then spago.
|
A few other thoughts:
|
|
Superseded by #1202 |
Description of the change
Continues #1154 to illustrate a way to approach the auto-migration problem. I figured I would open this as a separate PR to illustrate some changes before committing to them.