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

Fix OptionParser to handle sub-commands with hyphen #9465

Merged
merged 1 commit into from
Jun 23, 2020
Merged

Fix OptionParser to handle sub-commands with hyphen #9465

merged 1 commit into from
Jun 23, 2020

Conversation

erdnaxeli
Copy link
Contributor

It was matching regexs anywhere on the string, but we want the regexs to
match the full string. In particular this behavior made that a flag
sub-command was interpeted as su, because it matches the -(.)\S+
regex.

Copy link

@aravindavk aravindavk left a comment

Choose a reason for hiding this comment

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

This PR looks good to me. Any help required to get this Patch merged?

I also faced the same issue and sent patch #9481 Abandoned it after noticing this patch.

@erdnaxeli
Copy link
Contributor Author

erdnaxeli commented Jun 17, 2020

This PR looks good to me. Any help required to get this Patch merged?

I'm not sure what is the process to get reviewed and / or merged.

@aravindavk
Copy link

Based on the file history including @RX14 for review. Thanks.

@straight-shoota
Copy link
Member

This should better use \A and \z instead of ^ and $.
The difference may not be relevant for this use case because flags usually don't contain newline characters, but it's the right tool for this.

@erdnaxeli
Copy link
Contributor Author

erdnaxeli commented Jun 23, 2020

@straight-shoota I didn't know about this, it seems you're right: https://guides.rubyonrails.org/security.html#regular-expressions.

I fixed it, thanks. I didn't write test for it because I am wondering what we should expect when someone puts a newline in a flag.

@jhass jhass added this to the 1.0.0 milestone Jun 23, 2020
@straight-shoota
Copy link
Member

It should be lower-case \z, though.

It was matching regexs anywhere on the string, but we want the regexs to
match the full string. In particular this behavior made that a flag
`sub-command` was interpeted as `su`, because it matches the `-(.)\S+`
regex.
@erdnaxeli
Copy link
Contributor Author

Indeed, from ruby doc:

  • \Z - Matches end of string. If string ends with a newline, it matches just before newline
  • \z - Matches end of string

@bcardiff bcardiff changed the title Fix OptionParser.parse_flag_definition regexs Fix OptionParser to handle sub-commands with hyphen Jun 23, 2020
@bcardiff bcardiff merged commit 476486e into crystal-lang:master Jun 23, 2020
@erdnaxeli erdnaxeli deleted the fix_optionparser_subcommand branch June 24, 2020 08:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants