feat: Expose typedef edit actions in OpenAPI#949
Conversation
georgefst
force-pushed
the
georgefst/typedef-actions
branch
7 times, most recently
from
0e11bc5 to
17c9725
Compare
georgefst
force-pushed
the
georgefst/typedef-actions
branch
3 times, most recently
from
29155fc to
0695d8c
Compare
georgefst
changed the title
georgefst
force-pushed
the
georgefst/typedef-actions
branch
from
0695d8c to
96f2da8
Compare
georgefst
force-pushed
the
georgefst/typedef-actions
branch
5 times, most recently
from
73787bf to
0ec0742
Compare
georgefst
force-pushed
the
georgefst/typedef-actions
branch
2 times, most recently
from
f5cadd5 to
17dbeb3
Compare
| Just wrappedTy -> | ||
| f wrappedTy >>= \case | ||
| InType zt -> pure zt | ||
| z -> maybe unwrapError pure (focusType (unfocusLoc z)) |
There was a problem hiding this comment.
This whole function is a bit of a copy-paste from applyActionsToType, and I'm not certain that all details are relevant, or whether there's anything missing. Also, I don't know if there's anything from the recently-fixed SetConFieldType (#932) that we're missing. However, #1040 doesn't seem to be revealing any issues from forTypeDefConsFieldNode, so maybe we're in the clear.
There was a problem hiding this comment.
r.e. #932, I think this is fine: you are calling checkEverything instead of manually fixing all usages of this constructor.
There is some DRYing that could happen to withWrappedType in particular, but this could be done later.
There was a problem hiding this comment.
r.e. #932, I think this is fine: you are calling
checkEverythinginstead of manually fixing all usages of this constructor.
Ah, of course.
And indeed, given how general one of these actions can be, there didn't seem much alternative. But I suppose, now that you allude to it, we could in theory look for uses of the given constructor, and try to fix things more locally. I'm certainly not suggesting that we change that in this PR, however.
primer/src/Foreword.hs
Outdated
| findAndAdjustA' :: Applicative m => (a -> Bool) -> (a -> m (a, z)) -> [a] -> m (Maybe ([a], z)) | ||
| findAndAdjustA' p f = \case | ||
| [] -> pure Nothing | ||
| x : xs -> if p x then (\(x', z) -> Just . (,z) . (: xs) $ x') <$> f x else first (x :) <<$>> findAndAdjustA' p f xs |
There was a problem hiding this comment.
If these functions are staying (see above), I should at least add haddocks, and maybe pick a better name.
| . Just | ||
| . TypeDefConsNodeSelection | ||
| . TypeDefConsSelection con | ||
| $ Nothing |
There was a problem hiding this comment.
This might not be the selection we want. This means that when we add a field, the selection stays in the current constructor, rather than descending to the newly-created hole. The current approach has the advantage that it's easy to quickly add multiple fields, but it might feel inconsistent with what we do elsewhere.
If we do want to change this, I think we'll need something similar to the findAndAdjustA' machinery in applyActionsToField, for extracting the zipper and thus the ID of the hole.
There was a problem hiding this comment.
I'm not too bothered either way. I assume it would be easy to change if desired?
brprice
left a comment
brprice
left a comment
There was a problem hiding this comment.
Other than the issue with genNames, this looks good to me
Signed-off-by: George Thomas <georgefsthomas@gmail.com>
This will be necessary in order to make use of them in actions-related modules. Signed-off-by: George Thomas <georgefsthomas@gmail.com>
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 <georgefsthomas@gmail.com>
This will significantly decrease the amount of breakage when we unify our selection types. Signed-off-by: George Thomas <georgefsthomas@gmail.com>
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 <georgefsthomas@gmail.com>
Signed-off-by: George Thomas <georgefsthomas@gmail.com>
Signed-off-by: George Thomas <georgefsthomas@gmail.com>
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 <georgefsthomas@gmail.com>
Signed-off-by: George Thomas <georgefsthomas@gmail.com>
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 <georgefsthomas@gmail.com>
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 <georgefsthomas@gmail.com>
Signed-off-by: George Thomas <georgefsthomas@gmail.com>
Signed-off-by: George Thomas <georgefsthomas@gmail.com>
georgefst
force-pushed
the
georgefst/typedef-actions
branch
from
17dbeb3 to
f353179
Compare
It is of little use now that we have the more general `ConFieldAction`. Signed-off-by: George Thomas <georgefsthomas@gmail.com>
We can define these in terms of their un-primed equivalents, by choosing the correct applicative, so we effectively do so and then inline. Signed-off-by: George Thomas <georgefsthomas@gmail.com>
Signed-off-by: George Thomas <georgefsthomas@gmail.com>
georgefst
force-pushed
the
georgefst/typedef-actions
branch
from
f353179 to
c42e031
Compare
3 participants
This allows us to finally take advantage of #367 (with the caveats detailed in #399, #401 and #402).
This will ultimately enable us to render typedefs alongside terms in our frontend, and modify them using the same actions panel with which we manipulate terms.
See #1040 and #1041 for potential follow-ups.