Migrate updateName and addSources to use expr modifier #820
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Control flow before this commit would do all the lookup work and get the needed values, only to fail if the packages that are trying to be added don't exist in the package set. This commit does the check first.
In this commit, I moved the code that checks whether the user-requested packges exist in the package set into the `withRawConfigAST`, so that the new `Spago.Config.AST` module doesn't introduce a cyclial dependency with the `Spago.Config` module. I also provide both the raw and normalized AST expressions as it's the only way to correctly determine which packages haven't yet been installed.
This change enables us to pass the keystack back up to the let binding expression that defines the value for the binding. However, since the key stack might be empty, we can no longer use `NonEmpty Text`. Unfortunately, this change means `SearchingforField`'s arg must also be updated and by implication, `WithinField` and `SearchingForField` are conceptually the same thing.
To maintain the previous behavior, a let binding should only allow a `VariableName` to propagate to its parent expression if its own level is not an empty key stack (which means we're at the field we need to update). Otherwise, we will traverse outside of the current expression, and introduce a side-effect if we update the actual let binding's value.
To support the Right version of Project, we would need to traverse type-level constructors in the Dhall Expr type.
Previously, we were checking whether we should only update the `updateExpr` and only then falling back to updating either the `updateExpr` or the `recordExpr`. If the `updateExpr` was an exact match of the remaining fields we were seaching for, we would go down that path. Otherwise (i.e. the `updateExpr` was a partial match), we would try updating the `updateExpr` and then the `recordExpr` if the former failed By removing the "full" updateExpr case, we get the same execution but without the redundant code
While the immediate Var updates won't be as nice stylistically, they are always correct.
When 2+ config AST updates occur, the second one does not have access to the result of the first config update. Thus, the `isSemanticallyEquivalent` check fails because the second config update's expectec config does not contain the updates done in the first one. For example, if we update the name and then add dependencies, the dependencies ast update check does not have the new name in the 'expectedConfig' value. Moreover, in the process of doing this change, I discovered that I could remove the 'sortDependencies' part of the publishConfig check by just verifying that the keys in each maps are the same since that's all the error really cards about: missing keys.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implements what I proposed in this comment:
Since this builds off of #815, you can view this diff rather than the one presented in this PR. The first commit starts with the message, "Extract list text values in more modular way."
Also, I ran out of time today, so I'll have to see how the tests are affected by these changes later.
Here's an explanation of this PR. I migrated these by doing the following:
addRawDepsas that has been superceded byaddDependencies.updateNameso that it copiesaddDependenciesbut updates just the nameaddSourcePathsby defining a new config modification calledMigrateFromV1Configas that's howaddSourcePathswas used in this usageaddSourcePathsto not use it at all. Since thedependencieskey is always present in v1 configs and v2 configs, the usage was unnecessary.Ideally, the config modification updates would be batched together, so that the
spago.dhallfile is only updated once. This change does each atomically for simplicity. This is also likely why some tests will fail despite otherwise working correctly (e.g. error messages are different now).Remaining work:
MigrateFromV1Configshould be checked to verify it produces the expected config value if parsed. I haven't done that yet.