-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Better example program re-use in tests. #461
Conversation
05929c0
to
e70e297
Compare
e70e297
to
6a93cb3
Compare
8d491ba
to
ee3466e
Compare
5cca2a8
to
52cab05
Compare
mkASTDef
helper function.52cab05
to
92bd49d
Compare
There’s more I want to do to make program/app creation more ergonomic from Haskell, but this seems like a good place to cap off this particular PR. |
Note: this has been rebased on #497, so you can ignore the first few commits. |
{ appProg = testProg | ||
, appInit = NewApp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may be worth noting that this appInit
is a lie (and thus undo will not work with this App
). I suspect this also happens in other places we make a test App
. The lie is benign since we will never look at this field!
IIRC you had plans shortly to revisit how we encode the "initial app", which would fix this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that functionality is next in my queue. This PR is some groundwork for that.
"^Primer.Database.Rel8.Rel8Db.runRel8Db" | ||
] | ||
ignoreRoots = | ||
[ "^Primer.Core.Utils.mkASTDef" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit surprised you did not end up using this helper that has just been introduced
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Me too. In retrospect, I wouldn't have bothered with it, but this PR has been in flight for several weeks and sometimes that's just how development goes.
I thought about going back and rewriting everything from the start, knowing that I would not need it in the end, but that would require a lot of busy work, so I had 2 practical choices: add a final commit to remove it, now that it's no longer needed, or keep it around in case it's useful in the future. I chose the latter, but I can do the former if you prefer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds fine to me. A third choice is to use it when defining the comprehensive example, but that may be slightly awkward code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good. I had a few minor comments above. I'm happy for this to merge when you have looked at those.
92bd49d
to
214927a
Compare
This doesn't save much typing, but the `NonEmptyList` constructor name is annoyingly hard to remember.
We parameterize the `Primer.Examples` functions over module names, to make them easier to reuse in tests, example DSL programs, etc.
Previously, we used this test definition verbatim in 2 different places.
214927a
to
b93078f
Compare
No description provided.