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

refactor: Use DSL v2 types in place of ScannerDefinitionNode #1003

Merged
merged 6 commits into from
Jun 20, 2024

Conversation

Xanewok
Copy link
Contributor

@Xanewok Xanewok commented Jun 4, 2024

Based on #1002
Part of #638

The DSL v2's model::Scanner is almost 1:1 usable with our current parser codegen, with the exception of versioned scanners. In short we:

  • introduce a helper type VersionedScanner that is used in place of the old ScannerDefinitionNode::Versioned
  • introduce a ScannerExt trait implemented for both VersionedScanner and the model::Scanner, which is responsible for generating the main scanning logic
  • Repurposes ScannerDefinition slightly to surface more of the scanner-related logic for trivia/fragment/token
  • implements this directly for model::{TriviaItem,FragmentItem,TokenItem} and stores it in the v1 Grammar struct
  • similarly, uses model::KeywordItem directly in place of v1's KeywordScannerDefinition as they shared the same functionality

@Xanewok Xanewok requested a review from a team as a code owner June 4, 2024 16:25
Copy link

changeset-bot bot commented Jun 4, 2024

⚠️ No Changeset found

Latest commit: edc7447

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Collaborator

@OmarTawfik OmarTawfik left a comment

Choose a reason for hiding this comment

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

Left one suggestion. But I think @AntonyBlakey would be a better reviewer for the main changes here.

Copy link
Contributor

@AntonyBlakey AntonyBlakey left a comment

Choose a reason for hiding this comment

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

I'm not clear about the separation between Keyword and Scanner - one is a verb and one a noun, but I don't know if that is true about their roles? This split happened after I left the codebase, so it's my responsibility I know to work it out, but TBH the naming is becoming less self-describing and more an in-group code/shorthand.

@Xanewok
Copy link
Contributor Author

Xanewok commented Jun 16, 2024

@AntonyBlakey I've changed some names, let me know if that's okay now 🙏

@AntonyBlakey
Copy link
Contributor

Not 100% happy with the naming in this section of our codebase, but this PR is not the place for that more general argument, so I am approving this now.

@Xanewok Xanewok added this pull request to the merge queue Jun 20, 2024
Merged via the queue into NomicFoundation:main with commit 0aa29ad Jun 20, 2024
1 check passed
@Xanewok Xanewok deleted the use-v2-types-scanner branch June 20, 2024 14:56
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.

3 participants