search

idank / bashlex

Python parser for bash
GNU General Public License v3.0
550 stars 94 forks source link

yacc: fixed newline bug (fixes #47) #71

Closed BlankCanvasStudio closed 2 years ago

BlankCanvasStudio commented 2 years ago

This adds support for all the errors outlined in issue #47. This also fixes issues: #1, #2, #23, #32, and #55. Just added the edge case into yacc. Adding it to the grammar or to the tokenizer ends up being a lot of work than adding it directly to yacc (in both cases you need to add it to a lot of different edge situations versus basically just ignoring the token in yacc.py)

BlankCanvasStudio commented 2 years ago

I have no idea why the 2.7 version failed. I'm developing in 3 but the errors in 2 don't seem related to the \n bug, as none of the problem lines actually have a line break in them. Any insight as to why this is failing? Thankfully its the same test in all 3 cases but I have no idea whats the matter there.

idank commented 2 years ago

Will try to have a look in the next few days. Thanks for working on this!

lacraig2 commented 2 years ago

This PR fixes several issues I would have otherwise reported. Thanks @BlankCanvasStudio

idank commented 2 years ago

Sorry I haven't had time to look into this. @BlankCanvasStudio were you able to figure this out?

lacraig2 commented 2 years ago

I found the issue. This t == 3 value is checking for a NEWLINE action. The number 3 is a NEWLINE in python 3, but on python 2 it's a totally different action.

https://github.com/idank/bashlex/blob/5c8b6d67eeceaab50f869b6bc9f43d87ba5c48f8/bashlex/yacc.py#L374

I don't know how to handle this on github other than sending @BlankCanvasStudio a PR. https://github.com/BlankCanvasStudio/bashlex/pull/1

I opted to replace the hardcoded value with newline = actions[state].get('NEWLINE') at the top where state=0 as it seems to contain a lookup table of values. https://github.com/lacraig2/bashlex/blob/lookup_fix/bashlex/yacc.py#L347

@idank feel free to do whatever is easiest to get this fixed up. It may be somewhat annoying to get a PR approved into @BlankCanvasStudio's branch for this PR so you can either cherry-pick it, merge it, add the 2 lines yourself, or, I suppose, wait for that PR.

Thanks.

BlankCanvasStudio commented 2 years ago

I'll merge it into my branch and squash the commits into the proper format for the repo. I appreciate the help!

idank commented 2 years ago

Great work fellas!

How come we're not adding any tests? Seems like this solves a bunch of things, so I expected seeing new tests.

idank commented 2 years ago

Also do you mind mentioning all of the issues you're fixing in the commit message? e.g. ... (fixes #1, #2, .., #n).