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

Set contains_keywords flag for implicit gets($/, chomp: true) method to handle -l CLI option #3145

Merged
merged 1 commit into from
Oct 7, 2024

Conversation

andrykonchin
Copy link
Member

I suppose the contains_keywords flag is missing here because arguments contain keyword arguments chomp: true .

Examples before and after:

bin/prism parse -n -l -e 'puts $_.end_with?("\n")'

AST before:

@ ProgramNode (location: (1,0)-(1,0))
├── flags: ∅
├── locals: []
└── statements:
    @ StatementsNode (location: (1,0)-(1,0))
    ├── flags: ∅
    └── body: (length: 1)
        └── @ WhileNode (location: (1,0)-(1,0))
            ├── flags: newline
            ├── keyword_loc: (1,0)-(1,0) = ""
            ├── closing_loc: (1,0)-(1,0) = ""
            ├── predicate:
            │   @ CallNode (location: (1,0)-(1,0))
            │   ├── flags: ignore_visibility
            │   ├── receiver: ∅
            │   ├── call_operator_loc: ∅
            │   ├── name: :gets
            │   ├── message_loc: ∅
            │   ├── opening_loc: ∅
            │   ├── arguments:
            │   │   @ ArgumentsNode (location: (1,0)-(1,23))
            │   │   ├── flags: ∅
            │   │   └── arguments: (length: 2)
...

AST after:

@ ProgramNode (location: (1,0)-(1,0))
├── flags: ∅
├── locals: []
└── statements:
    @ StatementsNode (location: (1,0)-(1,0))
    ├── flags: ∅
    └── body: (length: 1)
        └── @ WhileNode (location: (1,0)-(1,0))
            ├── flags: newline
            ├── keyword_loc: (1,0)-(1,0) = ""
            ├── closing_loc: (1,0)-(1,0) = ""
            ├── predicate:
            │   @ CallNode (location: (1,0)-(1,0))
            │   ├── flags: ignore_visibility
            │   ├── receiver: ∅
            │   ├── call_operator_loc: ∅
            │   ├── name: :gets
            │   ├── message_loc: ∅
            │   ├── opening_loc: ∅
            │   ├── arguments:
            │   │   @ ArgumentsNode (location: (1,0)-(1,23))
            │   │   ├── flags: contains_keywords

@@ -21905,6 +21905,7 @@ wrap_statements(pm_parser_t *parser, pm_statements_node_t *statements) {
));

pm_arguments_node_arguments_append(arguments, (pm_node_t *) keywords);
pm_node_flag_set((pm_node_t *) arguments, PM_ARGUMENTS_NODE_FLAGS_CONTAINS_KEYWORDS);
Copy link
Member Author

Choose a reason for hiding this comment

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

Wondering if it's better to set this flag in pm_arguments_node_arguments_append

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's okay, I think this is fine here.

@@ -21905,6 +21905,7 @@ wrap_statements(pm_parser_t *parser, pm_statements_node_t *statements) {
));

pm_arguments_node_arguments_append(arguments, (pm_node_t *) keywords);
pm_node_flag_set((pm_node_t *) arguments, PM_ARGUMENTS_NODE_FLAGS_CONTAINS_KEYWORDS);
Copy link
Member Author

Choose a reason for hiding this comment

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

Also wondering if there is a convenient place for a test for this case - fixing an issue without adding tests seems wrong to me.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, could you put this in command_line_test.rb?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, will add the test.

@andrykonchin andrykonchin changed the title Set contains_keywords flag for implicit gets($/, chomp: true) method handle -l CLI option Set contains_keywords flag for implicit gets($/, chomp: true) method to handle -l CLI option Oct 7, 2024
Copy link
Collaborator

@kddnewton kddnewton left a comment

Choose a reason for hiding this comment

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

Approve, but if you don't mind adding a quick test that would be good

@@ -21905,6 +21905,7 @@ wrap_statements(pm_parser_t *parser, pm_statements_node_t *statements) {
));

pm_arguments_node_arguments_append(arguments, (pm_node_t *) keywords);
pm_node_flag_set((pm_node_t *) arguments, PM_ARGUMENTS_NODE_FLAGS_CONTAINS_KEYWORDS);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, could you put this in command_line_test.rb?

@andrykonchin andrykonchin force-pushed the ak/fix-handling-of-l-cli-option branch from 85fb122 to 717e41c Compare October 7, 2024 15:46
@kddnewton kddnewton merged commit c38df36 into ruby:main Oct 7, 2024
54 checks passed
@andrykonchin andrykonchin deleted the ak/fix-handling-of-l-cli-option branch October 7, 2024 16:19
@andrykonchin andrykonchin restored the ak/fix-handling-of-l-cli-option branch October 17, 2024 17:10
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