Skip to content
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

WIP: Add 4.08 Grammar #66

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft

Conversation

NathanReb
Copy link
Collaborator

No description provided.

@NathanReb
Copy link
Collaborator Author

@ceastlund turns out I already opened a WIP PR!

I followed your suggestions ie I added dummy 4.08 modules pointing to the unchanged latest_version.ml and then I upgraded latest_version.ml and wrote the appropriate migrations in version_4_07.ml.

I went for the dumbest approach when writing the node migration functions as I wasn't sure where I was heading. I only just recently started cleaning it up a bit but it's still fairly simple and we can probably factor a lot of code there.

One thing I noticed is that I always passed the same version to the to_node and of_node functions for a given migration "direction" so I extracted the migration type that I pass around instead of those functions. I'm wondering if there is any good reason to pass it different versions. If not we could probably remove them from that argument.

@ceastlund
Copy link
Collaborator

Good point about the version arguments. If you like I'll make a PR to remove those arguments.

@ceastlund
Copy link
Collaborator

For the test failures, it might help if we wrote some tests directly against the History interface. We could then test individual conversions, downgrade and upgrade, against both randomized and hard-coded inputs, and choose the hard-coded ones to stress each AST change. We'd want to test round-tripping, and that output satisfies the right grammar. I think these tests would be easier to use to get the conversions right, and the Ppx_ast tests would be more sanity checks.

Want to make a PR adding some tests and I'll take a look at it?

@NathanReb
Copy link
Collaborator Author

Yeah I'll try to add such tests!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants