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

Convert ContainedSpan to ContainedSpan<T> #187

Closed
wants to merge 7 commits into from

Conversation

JohnnyMorganz
Copy link
Collaborator

@JohnnyMorganz JohnnyMorganz commented Jun 28, 2021

Converts ContainedSpan to ContainedSpan<T>, and cleans up all the fields to do so.
This PR is quite big, with a lot of busywork, so a few highlights:

  • ContainedSpan is changed to ContainedSpan<T>, with a new field inner, and tokens split into start and end
  • ContainedSpan now properly implements Visit, VisitMut and Display
  • Some visitors which were explicitly declared because they were Enums with a ContainedSpan are now derived automatically
  • Derive macros are simplified: node(full_range) and visit(contains) are now removed

A few things to point out which I was not sure about:

  • TableConstructor and GenericDeclaration (Luau) now only have one field, should they be left as structs or converted to enums?
  • In some places we had 2 separate methods to retrieve the contained tokens (in a ContainedSpan) and the node contained inside. To somewhat make the change easier, I kept these two methods, but the method to retrieve the contained tokens now return a (&TokenReference, &TokenReference) rather than a ContainedSpan. Is it worth keeping these two methods separate, or just creating a new method which returns the ContainedSpan<T>? [Same issue with the corresponding "mutator" methods]

Still TODO:

  • Verify tests are still correct
  • Update changelog (quite a few breaking changes)
  • Check if the public API is still alright to use (will test this in a downstream consumer)

This PR also fixes #161 (supersedes #166)

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.

Node .tokens() is not correctly ordered for ContainedSpans
1 participant