Skip to content

Specify Optional(NonEmpty(List)) in v2 where we do in v1#668

Merged
Xanewok merged 2 commits intoNomicFoundation:mainfrom
Xanewok:optional-lists-in-v2
Nov 21, 2023
Merged

Specify Optional(NonEmpty(List)) in v2 where we do in v1#668
Xanewok merged 2 commits intoNomicFoundation:mainfrom
Xanewok:optional-lists-in-v2

Conversation

@Xanewok
Copy link
Copy Markdown
Contributor

@Xanewok Xanewok commented Nov 20, 2023

Part of #652

Let's unify these definitions to be in line with what's in v0/v1 as to
not change the definition and the CST shape once we switch to using v2
as our source of truth for the parser.

This is independent whether we will want that eventually or not. For
now, we should keep things consistent during the migration period. After
that, we can make a decision whether we want to keep these empty nodes
in the CST or not in the absence of any separated items.

Let's unify these definitions to be in line with what's in v0/v1 as to
not change the definition and the CST shape once we switch to using v2
as our source of truth for the parser.

This is independent whether we will want that *eventually* or not. For
now, we should keep things consistent during the migration period. After
that, we can make a decision whether we want to keep these empty nodes
in the CST or not in the absence of any separated items.
@Xanewok Xanewok requested a review from a team as a code owner November 20, 2023 18:51
@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Nov 20, 2023

⚠️ No Changeset found

Latest commit: 8b463ee

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

@Xanewok Xanewok mentioned this pull request Nov 20, 2023
9 tasks
Copy link
Copy Markdown
Contributor

@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.

looks like a few ones still have allow_empty = true. Is this intended?

@Xanewok Xanewok changed the title Specify Optional(Required(List)) in v2 where we do in v1 Specify Optional(NonEmpty(List)) in v2 where we do in v1 Nov 21, 2023
@Xanewok
Copy link
Copy Markdown
Contributor Author

Xanewok commented Nov 21, 2023

The three remaining allow_empty = true are separated as a bug-fix in #667 along with regression tests, while the ones changed are are just about not emitting the empty nodes and using the same definition as in v0/v1.

@Xanewok Xanewok requested a review from OmarTawfik November 21, 2023 12:27
@Xanewok Xanewok added this pull request to the merge queue Nov 21, 2023
Merged via the queue into NomicFoundation:main with commit a90a6fe Nov 21, 2023
@Xanewok Xanewok deleted the optional-lists-in-v2 branch November 21, 2023 22:11
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