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

Allow converting new ModuleDef to old TableSchema #1630

Merged
merged 2 commits into from
Aug 29, 2024

Conversation

kazimuth
Copy link
Contributor

@kazimuth kazimuth commented Aug 23, 2024

Description of Changes

This was not painful at all! The code is a mess but the tests pass, the paths commute! Gonna rewrite to be cleaner, but the logic is there.

It should be possible to go in reverse from TableSchema -> ModuleDef via RawModuleDefv8, I have not implemented that yet. Eventually we'll update TableSchema to look like v9 and then we can throw out that path.

API and ABI breaking changes

None

Expected complexity level and risk

1, this is tricky but I think I've got it right.

Testing

We test that this produces identical results for the old and the new path for all of the modules in modules (up to a small amount of sorting).

@kazimuth kazimuth requested a review from cloutiertyler August 23, 2024 23:53
@kazimuth kazimuth force-pushed the jgilles/v8validation branch from de74b8d to e9634c1 Compare August 26, 2024 17:09
@kazimuth kazimuth force-pushed the jgilles/newdef2schema branch from 0b26485 to e5413d8 Compare August 26, 2024 17:09
@kazimuth kazimuth force-pushed the jgilles/v8validation branch 2 times, most recently from a8e90e1 to 06a9e70 Compare August 27, 2024 19:00
@kazimuth kazimuth force-pushed the jgilles/newdef2schema branch 2 times, most recently from 901e2eb to 4e617bb Compare August 27, 2024 20:03
@kazimuth kazimuth force-pushed the jgilles/v8validation branch from 3ad5756 to 8834292 Compare August 28, 2024 16:30
@kazimuth kazimuth force-pushed the jgilles/newdef2schema branch from 4e617bb to c7a268b Compare August 28, 2024 16:31
Base automatically changed from jgilles/v8validation to master August 29, 2024 11:01
bananas

Clean up

Fix deprecation errors

Vanquish clippy
@kazimuth kazimuth force-pushed the jgilles/newdef2schema branch from c7a268b to 4db02b7 Compare August 29, 2024 11:03
@kazimuth kazimuth marked this pull request as ready for review August 29, 2024 11:10
@kazimuth
Copy link
Contributor Author

This is ready for review.

I've ended up just using #[allow(deprecated)] in a number of places using the old path, because most of those places are going to need to be retouched as we rewrite stuff to use the new APIs (#1648, #1649, #1650, #1651, #1652, #1653, #1654)

Copy link
Contributor

@cloutiertyler cloutiertyler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM. I'm sorry for all the mapping, but at least it makes you verify familiar with the old and new stuff.

@kazimuth kazimuth added this pull request to the merge queue Aug 29, 2024
Merged via the queue into master with commit 2be4215 Aug 29, 2024
9 checks passed
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