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

Added tests and modules for mixin, rule stmt and quantifier expressions #15

Merged
merged 9 commits into from
May 25, 2024
Merged

Added tests and modules for mixin, rule stmt and quantifier expressions #15

merged 9 commits into from
May 25, 2024

Conversation

Vishalk91-4
Copy link
Contributor

@Vishalk91-4 Vishalk91-4 commented May 23, 2024 •
edited
Loading

1. Does this PR affect any open issues?(Y/N) and add issue references (e.g. "fix #123", "re #123".):

  • N
  • Y
re #8

2. What is the scope of this PR (e.g. component or file name):

  • grammar.js
  • expr.txt
  • schema.txt
  • grammar.json
  • node-types.json
  • parser.c
  • parser.h

3. Provide a description of the PR(e.g. more details, effects, motivations or doc link):

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Other

This PR does the below tasks

  • Added the mixin_statement
  • Added Test Cases for rule_statement
  • Added modules of quantifier expression in support of PR Test Case Supplements  #11 and added it to parser
  • Added modules for optional attributes in support of PR feat: add optional expr #13 and added it to parser
  • Refined and added more test cases for quantifier expression
  • Added Protocol and Check in Schema with their module defination

4. Are there any breaking changes?(Y/N) and describe the breaking changes(e.g. more details, motivations or doc link):

  • N
  • Y

5. Are there test cases for these changes?(Y/N) select and add more details, references or doc links:

  • Unit test
  • Integration test
  • Benchmark (add benchmark stats below)
  • Manual test (add detailed scripts or steps below)
  • Other

@Peefy
Copy link
Contributor

Peefy commented May 24, 2024

PR confilct

@Vishalk91-4
Copy link
Contributor Author

PR confilct

Resolved PR conflict occuring

Peefy
Peefy reviewed May 24, 2024
View reviewed changes
test/corpus/expr.txt Outdated Show resolved Hide resolved
test/corpus/expr.txt Outdated Show resolved Hide resolved
@Peefy
Copy link
Contributor

Peefy commented May 24, 2024

Add some comments and CI failed.

Peefy
Peefy reviewed May 24, 2024
View reviewed changes
test/corpus/expr.txt Outdated
--------------------------------------------------------------------------------

(module
(ERROR
Copy link
Contributor

Choose a reason for hiding this comment

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

unexpected (ERROR token.

test/corpus/expr.txt Outdated
(integer)
(integer)
(integer)))
(MISSING _newline)
Copy link
Contributor

Choose a reason for hiding this comment

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

unexpected (ERROR token.

test/corpus/expr.txt Outdated
(integer)))
(MISSING _newline)
(dictionary
(ERROR
Copy link
Contributor

Choose a reason for hiding this comment

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

unexpected (ERROR token.

test/corpus/expr.txt Outdated
--------------------------------------------------------------------------------

(module
(ERROR
Copy link
Contributor

Choose a reason for hiding this comment

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

unexpected (ERROR token.

test/corpus/expr.txt Outdated
(string_start)
(string_content)
(string_end))))))
(MISSING _newline)
Copy link
Contributor

Choose a reason for hiding this comment

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

unexpected (ERROR token.

test/corpus/expr.txt Outdated
(assignment
(dotted_name
(identifier))
(ERROR
Copy link
Contributor

Choose a reason for hiding this comment

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

unexpected (ERROR token.

test/corpus/expr.txt Outdated
(string_start)
(string_content)
(string_end))))))
(MISSING _newline)
Copy link
Contributor

Choose a reason for hiding this comment

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

unexpected (ERROR token.

test/corpus/expr.txt Outdated
(string_end))))))
(MISSING _newline)
(dictionary
(ERROR
Copy link
Contributor

Choose a reason for hiding this comment

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

unexpected (ERROR token.

@Vishalk91-4
Copy link
Contributor Author

Vishalk91-4 commented May 24, 2024 •
edited
Loading

Will fix all unwanted ERROR codes
And push a commit

@Vishalk91-4
Copy link
Contributor Author

@Peefy
I found out that the previous PRs merged did not run tree-sitter generate while adding things to grammar.js and so none of them were parsed, they were just added as ERROR token or something else.
And directly ran the tree-sitter test which also had a lot of ERROR tokens in it and passed it

I'll fix them all and create a commit soon here

@Vishalk91-4
Copy link
Contributor Author

Vishalk91-4 commented May 24, 2024 •
edited
Loading

@Peefy

The previous PR #14 merged, added number_bin_suffix_expr, and so now for most of test cases, I need to change integer to this number_bin_suffix_expr and list to braces_exprerssion
Don't you think it would cause difficulty in understanding as integer, list previously was much readable when seen in tests

@Peefy
Copy link
Contributor

Peefy commented May 24, 2024

@Peefy

The previous PR #14 merged, added number_bin_suffix_expr, and so now for most of test cases, I need to change integer to this number_bin_suffix_expr and list to braces_exprerssion Don't you think it would cause difficulty in understanding as integer, list previously was much readable when seen in tests

I think integer and list are better.

@Vishalk91-4
Copy link
Contributor Author

@Peefy
The previous PR #14 merged, added number_bin_suffix_expr, and so now for most of test cases, I need to change integer to this number_bin_suffix_expr and list to braces_exprerssion Don't you think it would cause difficulty in understanding as integer, list previously was much readable when seen in tests

I think integer and list are better.

So do you want me to remove the number_bin_suffix_expr, as it's presence will force me to add it in place of integer for test cases

@Peefy
Copy link
Contributor

Peefy commented May 24, 2024

@Peefy
The previous PR #14 merged, added number_bin_suffix_expr, and so now for most of test cases, I need to change integer to this number_bin_suffix_expr and list to braces_exprerssion Don't you think it would cause difficulty in understanding as integer, list previously was much readable when seen in tests

I think integer and list are better.

So do you want me to remove the number_bin_suffix_expr, as it's presence will force me to add it in place of integer for test cases

Yes.

@Vishalk91-4
Copy link
Contributor Author

@Peefy
I was just asking, did we need that most recent PR Merged in dotted_name: $ => prec(1, sep1($.identifier, choice('?.','.',))), when we already have attribute and optonal_attribute and optional_item

@Peefy
Copy link
Contributor

Peefy commented May 24, 2024

No

@Vishalk91-4
Copy link
Contributor Author

No

So do you want me to keep that change of dotted_name: $ => prec(1, sep1($.identifier, choice('?.','.',))), or remove them and we can use the optional_attribute and optional_item

@Vishalk91-4
Copy link
Contributor Author

@Peefy
Could you review it now, I have removed the Error codes in most I guess

Peefy
Peefy approved these changes May 25, 2024
View reviewed changes
Copy link
Contributor

@Peefy Peefy left a comment

Choose a reason for hiding this comment

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

LGTM

@Peefy Peefy merged commit ea49b21 into kcl-lang:main May 25, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Peefy Peefy Peefy 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
@Vishalk91-4 @Peefy