Fixing up #1040 #1065
Closed
Fixing up #1040 #1065
Conversation
This file contains 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
Signed-off-by: George Thomas <[email protected]>
This will be necessary in order to make use of them in actions-related modules. Signed-off-by: George Thomas <[email protected]>
We parameterise this way, rather than parameterising `ExprMeta` and `TypeMeta` separately, since we otherwise wouldn't be able to reconstruct even a basic `Selection' ID` from the OpenAPI API, without clients knowing whether a particular ID corresponds to a type or term. We want clients to be able to be dumber than that. Note that, for `Selection`, we use a synonym for the non-parameterised version, but not for `NodeSelection`, and we will not in future for `DefSelection` and `TypeDefSelection`. That's because this synonym is actually widely useful, and we use it in several modules to make things more readable. With `NodeSelection` etc. we never really require such a synonym, and we avoid it because of all the ceremony it would add, particularly around imports. Signed-off-by: George Thomas <[email protected]>
This will significantly decrease the amount of breakage when we unify our selection types. Signed-off-by: George Thomas <[email protected]>
This removes some boilerplate where we converted between types which are essentially the same. That boilerplate would have become much bigger once we extend selections to cover type definitions. Despite the previous commit, there is one small breaking change, seen in `openapi.json` (`meta` instead of `id`). Signed-off-by: George Thomas <[email protected]>
Signed-off-by: George Thomas <[email protected]>
Signed-off-by: George Thomas <[email protected]>
This will make it easier to output typedefs in the API, due to increased uniformity with AST typedefs. Note that these names aren't even currently used anywhere since we don't actually have any higher-kinded primitives, only ints and chars. These would be used if we added, for example, `IO` or `Array` primitives, in which case the names would be likely useful at least for _displaying_ primitive typedefs, even though they don't actually scope over anything like they do for ASTs. Signed-off-by: George Thomas <[email protected]>
Signed-off-by: George Thomas <[email protected]>
Note that no new unit tests are added. This is because the underlying `ProgAction`s are already well-tested, e.g. in `unit_RenameType`. In due course, the property test `tasty_available_actions_accepted` will be generalised to cover typedef actions, which will therefore check that all of these new actions can be applied without error whenever they are available. We don't yet expose actions for constructor fields (i.e. `Available.forTypeDefConsFieldNode` always returns `[]`), since making this work will, unlike the other positions, require changes to the core of the library. We _could_ actually make all the `for*`s part of one definition, now that `Selection` is in `App.Base`. But we do actually use the individual functions in some tests, and with them separate, we have slightly more control, in that we don't need to provide as much context. Note that most of the changes in this commit are actually knock-on effects of generalising `Selection` to cover type defs. Signed-off-by: George Thomas <[email protected]>
This was motivated by a surprising deep equality failure on selections in a TypeScript frontend, due to a null field only present in one of the compared values. Signed-off-by: George Thomas <[email protected]>
Signed-off-by: George Thomas <[email protected]>
Signed-off-by: George Thomas <[email protected]>
This will be necessary when we extend to typedefs, since we need to branch here, but can't use `label` in `GenT`. Signed-off-by: George Thomas <[email protected]>
This should have been done back when `forDef` etc. acquired their current names. Signed-off-by: George Thomas <[email protected]>
Signed-off-by: George Thomas <[email protected]>
Signed-off-by: George Thomas <[email protected]>
Signed-off-by: George Thomas <[email protected]>
Signed-off-by: George Thomas <[email protected]>
we never consistently checked these things in `applyProgAction`, and it would be awkward to do so. besides, why do we care? Signed-off-by: George Thomas <[email protected]>
-- TODO rename `updateType` to `updateTypeDef` and this to `updateType`? in all branches? Signed-off-by: George Thomas <[email protected]>
Signed-off-by: George Thomas <[email protected]>
Signed-off-by: George Thomas <[email protected]>
Signed-off-by: George Thomas <[email protected]>
…w fails for different reason
I now understand your comment about needing the local cxt! I don't know why it was implemented in this way in the first place, and it seems rather silly given that we now (I changed this a few commits ago) extract the cached chkedAt type below!
Signed-off-by: Drew Hess <[email protected]>
Signed-off-by: Drew Hess <[email protected]>
The test fails to find the `LocalVarRef "a37"`. Signed-off-by: Drew Hess <[email protected]>
|
I thought I'd take a shot at rebasing this, but before I even got to that point, just focusing on cleaning up #1040 as-is, at least one functional test is failing, as well as a bunch of (all?) the benchmark sanity checks, so I'm probably out of my depth here. |
|
Thanks, I've carried your fixes back over to #1040. |
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.
No description provided.