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

Update parser.py #19

Merged
merged 3 commits into from
Jul 26, 2024
Merged

Update parser.py #19

merged 3 commits into from
Jul 26, 2024

Conversation

Cyerunix
Copy link
Contributor

@Cyerunix Cyerunix commented Jul 6, 2024

In the parse function in parser/parser.py, if the empty string is included in parsed_elements_list (which may be the case when parsing commands such as echo ""), the function call to are_all_individually_flags on line 39 will fail because once line 117 is reached (the first line of are_all_individually_flags), the code will attempt to access the first character of potential_flag_or_option, which will fail since potential_flag_or_option will be the empty string at this point.

I believe this slight change should fix the problem.

Copy link

github-actions bot commented Jul 6, 2024

OS:ubuntu-20.04
Sat Jul 6 02:25:30 UTC 2024
intro: 2/2 tests passed.
interface: 41/41 tests passed.
compiler: 54/54 tests passed.

@angelhof
Copy link
Member

Thank you for this PR! Could you also add a test that checks this behavior? Then we can merge it!

Copy link

OS:ubuntu-20.04
Fri Jul 12 19:39:27 UTC 2024
intro: 2/2 tests passed.
interface: 41/41 tests passed.
compiler: 54/54 tests passed.

Copy link

OS:ubuntu-20.04
Fri Jul 12 20:13:45 UTC 2024
intro: 2/2 tests passed.
interface: 41/41 tests passed.
compiler: 54/54 tests passed.

@Cyerunix
Copy link
Contributor Author

This test script works for the current implementation, though I'm not sure it is the best solution as it interprets code such as echo "" as though the quotes are the operand, so when you print out the CommandInvocationInitial object, it appears as though the operand_list is empty, but it actually has a single element Operand(""), which appears as nothing when printed. I believe this to be confusing and deceptive (and potentially unnecessary) behavior, but I uploaded the tests with no additional changes to the code regardless in case you disagree with me on this point. I will gladly provide a fix that prevents any Operand("") instances from being included in the operand_list if that is what you would prefer.

@angelhof
Copy link
Member

@Cyerunix Thanks and sorry for the delayed response! As far as I understand the operand is now correctly parsed but it is not correctly printed. If that is the case I think we can safely merge and this can be fixed later. If I misunderstood something let me know!

@Cyerunix
Copy link
Contributor Author

@Cyerunix Thanks and sorry for the delayed response! As far as I understand the operand is now correctly parsed but it is not correctly printed. If that is the case I think we can safely merge and this can be fixed later. If I misunderstood something let me know!

I believe what happens is the printer removes the quotes around operands when it prints them, so something like Operand("Hello") will simply be printed as Hello. However, when we have Operand(""), the quotes are still removed, leaving us with just , which I, again, believe is very unclear since it looks the same as having nothing at all. So, if we want to continue parsing "" as an operand, then my pull request should be fine to merge, and the printer should ideally be modified to handle this case. Otherwise, this PR will need to be changed to instead cause the code to ignore any instances of "" as an operand (so echo "" will be parsed as having 0 operands rather than 1, for example). If you believe treating "" as a valid operand is fine, then this PR is fine to merge and the printing ambiguity can be handled later.

@angelhof
Copy link
Member

Thanks for the clarification @Cyerunix !! Yes it is correct to parse "" as an operand, so the printing needs to be fixed later. If you can add an issue about it pointing to this PR too that would be great!!

@angelhof angelhof merged commit 2c06fb4 into binpash:main Jul 26, 2024
2 checks passed
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