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

Interpret field_or_op before sub_expression #22

Merged
merged 1 commit into from
Nov 4, 2024

Conversation

acerbusace
Copy link
Collaborator

Change

Interpret field_or_op before sub_expression. More details here.

Copy link
Collaborator

@myronmarston myronmarston left a comment

Choose a reason for hiding this comment

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

💯 Looking good! To get a green build I think you'll need to rebase once #23 is merged.

@@ -103,6 +103,8 @@ def filters_on_sub_fields?(expression)
end

def process_not_expression(bool_node, expression, field_path)
return if expression.nil? || expression == {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

According to the RBS signature, expression cannot be nil. Before now that was true but with your refactoring it can now be nil.

Can you update the RBS signatures for these methods to account for the new possible values?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@acerbusace acerbusace force-pushed the block/alexp/identifyNodeTypeChange branch 2 times, most recently from 9289923 to 03ece85 Compare November 4, 2024 20:16
@acerbusace acerbusace enabled auto-merge (squash) November 4, 2024 20:17
@acerbusace acerbusace disabled auto-merge November 4, 2024 20:39
@acerbusace acerbusace force-pushed the block/alexp/identifyNodeTypeChange branch from 03ece85 to 5dc50df Compare November 4, 2024 20:39
@acerbusace acerbusace enabled auto-merge (squash) November 4, 2024 20:39
@acerbusace acerbusace merged commit 47c438b into main Nov 4, 2024
10 checks passed
@acerbusace acerbusace deleted the block/alexp/identifyNodeTypeChange branch November 4, 2024 20:58
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