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.

Sign up for GitHub

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

Jump to bottom

Updated the grammar rules to allow for optional trailing commas. #1510

Open
wants to merge 24 commits into
base: main
Choose a base branch
from
Open

Updated the grammar rules to allow for optional trailing commas. #1510

wants to merge 24 commits into from

Conversation

MahtabNorouzi
Copy link
Collaborator

  • Tests added for any new code
  • Documentation added for any new functionality
  • Entries added to the respective CHANGELOG.md for any new functionality
  • Feature table on README.md updated for any listed functionality

bugarela
bugarela reviewed Sep 23, 2024
View reviewed changes
Copy link
Collaborator

@bugarela bugarela left a comment

Choose a reason for hiding this comment

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

We should add a test for this, such as adding some usages of all new forms of trailing commas to SuperSpec.qnt - and then updating SuperSpec.json by running npm run update-fixtures

quint/src/generated/Quint.g4 Outdated Show resolved Hide resolved
@MahtabNorouzi MahtabNorouzi changed the title fix: enforce trailing comma in parameter list for grammar rules ### Summary of Changes - Updated the grammar rules to allow for optional trailing commas in function parameter definitions. - Modified the min function in superspec.map.json to demonstrate this change while keeping the add function unchanged for reference. - Added an import statement: import Proto(N = N,) as Inst1. ### Details - The min function now accepts parameters in the following formats: - (x: int, y: int,) - (x: int, y: int) - The import statement is included to utilize the Proto module with an optional trailing comma. ### Reason for Changes - Allowing an optional trailing comma makes the syntax more flexible and consistent with common programming practices. ### Test Cases - Added test cases for both scenarios in superspec.map.json to ensure correct parsing of functions with optional trailing commas. Sep 23, 2024
@MahtabNorouzi MahtabNorouzi changed the title ### Summary of Changes - Updated the grammar rules to allow for optional trailing commas in function parameter definitions. - Modified the min function in superspec.map.json to demonstrate this change while keeping the add function unchanged for reference. - Added an import statement: import Proto(N = N,) as Inst1. ### Details - The min function now accepts parameters in the following formats: - (x: int, y: int,) - (x: int, y: int) - The import statement is included to utilize the Proto module with an optional trailing comma. ### Reason for Changes - Allowing an optional trailing comma makes the syntax more flexible and consistent with common programming practices. ### Test Cases - Added test cases for both scenarios in superspec.map.json to ensure correct parsing of functions with optional trailing commas. fix: Updated the grammar rules to allow for optional trailing commas in function parameter definitions. Sep 23, 2024
@MahtabNorouzi MahtabNorouzi changed the title fix: Updated the grammar rules to allow for optional trailing commas in function parameter definitions. fix: Updated the grammar rules to allow for optional trailing commas. Sep 23, 2024
@MahtabNorouzi MahtabNorouzi changed the title fix: Updated the grammar rules to allow for optional trailing commas. Updated the grammar rules to allow for optional trailing commas. Sep 23, 2024
@MahtabNorouzi
Copy link
Collaborator Author

Summary of Changes

  • Updated the grammar rules to allow for optional trailing commas.
  • Modified the min function in superspec.map.json to demonstrate this change while keeping the add function unchanged for reference.
  • Added an import statement: import Proto(N = N,) as Inst1.

Details

  • The min function now accepts parameters in the following formats:
    • (x: int, y: int,)
    • (x: int, y: int)
  • The import statement is included to utilize the Proto module with an optional trailing comma.

Reason for Changes

  • Allowing an optional trailing comma makes the syntax more flexible and consistent with common programming practices.

Test Cases

  • Added test cases for both scenarios in superspec.map.json to ensure correct parsing of functions with optional trailing commas.

bugarela
bugarela approved these changes Sep 24, 2024
View reviewed changes
Copy link
Collaborator

@bugarela bugarela left a comment

Choose a reason for hiding this comment

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

Looks great! Before merging it, can you add "Closes #1383" to the PR description, so the issue gets automatically closed when we merge this?

@bugarela
Copy link
Collaborator

Actually, I remembered something else: we should add an entry to CHANGELOG.md saying that we now allow trailing commas in operator definitions and constant initialization - which also makes me think we might have missed one case: operator call. We should be able to call min(1, 2,) just as well as we can define min(x: int, y: int,)

bugarela
bugarela reviewed Oct 3, 2024
View reviewed changes
CHANGELOG.md Outdated

- Updated grammar rule to allow an optional trailing comma in parameter lists:

- Function calls
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- Function calls
- Operator calls

bugarela
bugarela approved these changes Oct 3, 2024
View reviewed changes
Copy link
Collaborator

@bugarela bugarela left a comment

Choose a reason for hiding this comment

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

Looks great! I've just merged another PR that touches the grammar, so you'll need to re-generate the files once merging main on this - sorry about that! Otherwise, we're good to go ✨

CHANGELOG.md Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bugarela bugarela bugarela approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@MahtabNorouzi @bugarela