Introduce negative numbers parsing #342
Conversation
WalkthroughWalkthroughThe changes introduce a new negative prefix operator in the Shield project to handle negative expressions and integers. This enhancement involves updating various components, including the parser, evaluator, and abstract syntax tree (AST), to correctly parse, evaluate, and process negative numbers. Additionally, new test cases ensure the functionality works as intended. Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant User
participant REPL
participant Parser
participant Evaluator
participant AST
participant Metadata
participant Preprocessor
User->>REPL: Enter expression with negative number
REPL->>Parser: Parse expression
Parser->>AST: Create PrefixExpression node
Parser->>Preprocessor: Preprocess PrefixExpression
Preprocessor->>AST: Process right side of PrefixExpression
AST->>Evaluator: Evaluate PrefixExpression
Evaluator->>AST: Evaluate right side of PrefixExpression
Evaluator->>Metadata: Update metadata
Metadata->>Evaluator: Return updated metadata
Evaluator->>REPL: Return evaluated result
REPL->>User: Display result
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (3)
shield/internal/metadata/metadata.go (1)
36-37: The handling ofPrefixExpressionin metadata processing is correctly implemented. Consider adding a comment explaining why only the right side of the prefix expression is processed.shield/internal/metadata/metadata_test.go (1)
78-82: The new test case for prefix expressions in metadata extraction is correctly implemented. Consider adding more comprehensive tests to cover complex nested expressions.CHANGELOG.md (1)
Line range hint
58-58: Correct the indentation for unordered list items to comply with markdown best practices.- * (shield) Add new operators: `<`, `>`, `<=`, `>=`, `==`, `!=` for comparing integers to each others. + * (shield) Add new operators: `<`, `>`, `<=`, `>=`, `==`, `!=` for comparing integers to each other.Also applies to: 74-74, 86-87, 111-113, 118-118, 121-121, 132-132, 137-138
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (2)
api/shield/ast/ast.pulsar.gois excluded by!api/**shield/ast/ast.pb.gois excluded by!**/*.pb.go
Files selected for processing (11)
- CHANGELOG.md (1 hunks)
- cmd/shield_repl/main.go (1 hunks)
- proto/shield/ast/ast.proto (2 hunks)
- shield/ast/ast.go (2 hunks)
- shield/internal/evaluator/evaluator.go (2 hunks)
- shield/internal/evaluator/evaluator_test.go (2 hunks)
- shield/internal/metadata/metadata.go (1 hunks)
- shield/internal/metadata/metadata_test.go (1 hunks)
- shield/internal/parser/parser.go (3 hunks)
- shield/internal/parser/parser_test.go (1 hunks)
- shield/internal/preprocess/preprocess.go (2 hunks)
Files skipped from review due to trivial changes (1)
- shield/internal/evaluator/evaluator_test.go
Additional context used
Path-based instructions (9)
cmd/shield_repl/main.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.shield/internal/metadata/metadata.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.shield/internal/preprocess/preprocess.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.shield/internal/metadata/metadata_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"shield/ast/ast.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.shield/internal/parser/parser_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"shield/internal/parser/parser.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.shield/internal/evaluator/evaluator.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
Markdownlint
CHANGELOG.md
58-58: Expected: 2; Actual: 4
Unordered list indentation
74-74: Expected: 2; Actual: 4
Unordered list indentation
86-86: Expected: 2; Actual: 4
Unordered list indentation
87-87: Expected: 2; Actual: 4
Unordered list indentation
111-111: Expected: 2; Actual: 4
Unordered list indentation
112-112: Expected: 2; Actual: 4
Unordered list indentation
113-113: Expected: 2; Actual: 4
Unordered list indentation
118-118: Expected: 2; Actual: 4
Unordered list indentation
121-121: Expected: 2; Actual: 4
Unordered list indentation
41-41: Expected: 1; Actual: 2
Multiple consecutive blank lines
132-132: Expected: 1; Actual: 2
Multiple consecutive blank lines
137-137: Expected: 1; Actual: 2
Multiple consecutive blank lines
138-138: Expected: 1; Actual: 3
Multiple consecutive blank lines
118-118: null
Bare URL used
Additional comments not posted (9)
cmd/shield_repl/main.go (1)
13-42: The REPL integration looks well-implemented and correctly utilizes the new parsing and evaluation functionalities.shield/internal/preprocess/preprocess.go (1)
17-18: The addition ofpreprocessPrefixExpressioncorrectly extends the preprocessing logic to handle prefix expressions. The implementation is consistent with the handling of other expression types.Also applies to: 37-41
proto/shield/ast/ast.proto (1)
19-19: The definition ofPrefixExpressionin the protobuf file is correctly implemented and essential for the AST representation of the new prefix expressions.Also applies to: 61-65
shield/ast/ast.go (1)
43-47: The implementation ofNewPrefixExpressionandUnwrapPrefixExpressionis correct and essential for the manipulation of prefix expressions within the AST.Also applies to: 96-101
shield/internal/parser/parser_test.go (1)
103-114: The test for negative integers appears comprehensive and correctly checks the parsing of a negative integer as a prefix expression.shield/internal/parser/parser.go (2)
71-71: Correctly registered the prefix parse function for handling the subtraction token as a prefix operator.
188-198: The implementation ofparsePrefixExpressioncorrectly sets up the prefix expression and recursively parses the right-hand side expression, adhering to the precedence rules.shield/internal/evaluator/evaluator.go (1)
41-42: The implementation ofevalPrefixExpressionandevalPrefixSubOperatorcorrectly handles the evaluation of prefix expressions, including the special case for the negative operator.Also applies to: 73-96
CHANGELOG.md (1)
51-51: The changelog entry for the addition of the negative prefix operator is clear and informative.
There was a problem hiding this comment.
Looking good!
I'll ask you to do a couple more little things but all the work looks good:
- add support for PrefixExpression to
stringify.go - add more test cases to
TestParses(see comment) so we can see that priority among operators (InfixExpression, CallExperssion) is correct and stays like that
| require.True(t, ok, "expression is not *ast.PrefixExpression. got=%T", expression) | ||
| require.Equal(t, "-", negative.Operator) | ||
| } | ||
|
|
||
| func TestParser(t *testing.T) { |
There was a problem hiding this comment.
can we add some tests here to ensure that the priority of operators is the expected one?
7f8d5ab to
dfa9c4d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
CHANGELOG.md (1)
Line range hint
58-58: There are several markdown lint issues related to incorrect list indentation and multiple consecutive blank lines. Correcting these will improve the readability and maintain the standard formatting of the document.- * (shield) Add new operators: `<`, `>`, `<=`, `>=`, `==`, `!=` for comparing integers to each others. + * (shield) Add new operators: `<`, `>`, `<=`, `>=`, `==`, `!=` for comparing integers to each other.Also applies to: 75-75, 87-88, 112-114, 119-122, 133-133, 138-139
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (2)
api/shield/ast/ast.pulsar.gois excluded by!api/**shield/ast/ast.pb.gois excluded by!**/*.pb.go
Files selected for processing (12)
- CHANGELOG.md (1 hunks)
- cmd/shield_repl/main.go (1 hunks)
- proto/shield/ast/ast.proto (2 hunks)
- shield/ast/ast.go (2 hunks)
- shield/ast/stringify.go (1 hunks)
- shield/internal/evaluator/evaluator.go (2 hunks)
- shield/internal/evaluator/evaluator_test.go (2 hunks)
- shield/internal/metadata/metadata.go (1 hunks)
- shield/internal/metadata/metadata_test.go (1 hunks)
- shield/internal/parser/parser.go (3 hunks)
- shield/internal/parser/parser_test.go (2 hunks)
- shield/internal/preprocess/preprocess.go (2 hunks)
Files skipped from review as they are similar to previous changes (10)
- cmd/shield_repl/main.go
- proto/shield/ast/ast.proto
- shield/ast/ast.go
- shield/internal/evaluator/evaluator.go
- shield/internal/evaluator/evaluator_test.go
- shield/internal/metadata/metadata.go
- shield/internal/metadata/metadata_test.go
- shield/internal/parser/parser.go
- shield/internal/parser/parser_test.go
- shield/internal/preprocess/preprocess.go
Additional context used
Path-based instructions (2)
shield/ast/stringify.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
LanguageTool
CHANGELOG.md
[uncategorized] ~46-~46: The grammatical number of this noun doesn’t look right. Consider replacing it.
Context: ...tocol/225) Fix shield to handle grouped expression * (shield) Add new operators:<,>,...
[uncategorized] ~111-~111: The grammatical number of this noun doesn’t look right. Consider replacing it.
Context: ...ndx/warden's store design to improve queries performance: * (x/intent) [#117](ht...
[uncategorized] ~114-~114: You might be missing the article “the” here.
Context: ...otocol/wardenprotocol/pull/122) ImproveSpacesByOwnerquery by adding a new "ow...
Markdownlint
CHANGELOG.md
58-58: Expected: 2; Actual: 4
Unordered list indentation
75-75: Expected: 2; Actual: 4
Unordered list indentation
87-87: Expected: 2; Actual: 4
Unordered list indentation
88-88: Expected: 2; Actual: 4
Unordered list indentation
112-112: Expected: 2; Actual: 4
Unordered list indentation
113-113: Expected: 2; Actual: 4
Unordered list indentation
114-114: Expected: 2; Actual: 4
Unordered list indentation
119-119: Expected: 2; Actual: 4
Unordered list indentation
122-122: Expected: 2; Actual: 4
Unordered list indentation
41-41: Expected: 1; Actual: 2
Multiple consecutive blank lines
133-133: Expected: 1; Actual: 2
Multiple consecutive blank lines
138-138: Expected: 1; Actual: 2
Multiple consecutive blank lines
139-139: Expected: 1; Actual: 3
Multiple consecutive blank lines
119-119: null
Bare URL used
Additional comments not posted (2)
shield/ast/stringify.go (1)
27-28: The implementation forExpression_PrefixExpressionin theStringifyfunction correctly formats prefix expressions. This aligns well with the PR's objective to handle negative numbers and other prefix operations.CHANGELOG.md (1)
51-51: The changelog entry for the addition of the negative prefix operator is clear and appropriately categorized under "Consensus Breaking Changes". This ensures users are aware of the significant update in functionality.
|
It's been a pleasure to contribute! 😄 |
Hello, Warden Team 👋🏻
I'd like to contribute to this project by opening a PR to make the shield DSL handle negative integers. I didn't comment on the related issue before picking it up since I briefly chatted with @Pitasi about this. Please forgive me if this was not the expected workflow to follow.
The current PR defines the abstraction to handle the negative sign as a prefix expression (with the highest priority between the operators) instead of changing the lexer to manually recognize negative integers. This way, you can write more complex expressions (e.g.
--1,-(-1),-var[when/if constants/variables will be available]) without additional costs.Also, I used a small REPL tool to manually test the feature (as an additional step to write unit tests), and I decided to include it as part of the current PR. I'm happy to drop it if you don't think this might be useful.
I'm happy to discuss the proposed changes. Let me know if anything does not match what you expect. Thanks!
Closes #328
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation
Refactor