search

knrafto / language-bash

Parse and pretty-print Bash shell scripts
BSD 3-Clause "New" or "Revised" License
35 stars 9 forks source link

List assignments #13

Closed pbiggar closed 8 years ago

pbiggar commented 8 years ago

Assignments where the rvalue was an empty array didn't work. This adds some unit tests, including a test that that works.

I'm not sure if the fix is correct, but I tried lots of things to figure this out. Basically, the grammar for Assign is correct, but the operators stuff has higher precedence. If you have a better fix I'm happy to implement it.

pbiggar commented 8 years ago

This fix is definitely wrong (function definitions no longer parse). Would love advice on how to solve it properly.

knrafto commented 8 years ago

Here's where assignments are parsed: https://github.com/knrafto/language-bash/blob/master/src/Language/Bash/Parse/Word.hs#L329

pbiggar commented 8 years ago

Thanks for the link. I realize I forgot to let you know what I've done so far and to provide context - I didn't mean to imply I was too lazy to look at the code.

I did spend quite a bit of time last week looking at the assignment parser. After minimizing the grammar and playing around with a bunch of different examples, I believe that the assignment parser works just fine, but isn't being used. You can see this by using the code in the PR: if you remove "(" and ")" from the list of operators, then the assignment parser works perfectly.

However, after that I am stumped. I'm new to Haskell and Parsec and combinator based parsing (previous parsing experience is in Bison, Antlr and Spirit). I feel there might be a precedence issue here, but I really don't know what to do with that.

Any pointers you have would be much appreciated. Thanks!

pbiggar commented 8 years ago

Hey, just wanted to follow up and see if you had any other thoughts on how to address this issue?

knrafto commented 8 years ago

Really sorry for the delay - busy week.

Looks like the issue is that the parser thinks a=() is trying to define a function named a= (that's why it says it expects a {). It's an easy fix, here's a patch (I can't push to your branch): http://pastebin.com/vVwncmuW

Thanks a lot for the unit tests!

pbiggar commented 8 years ago

Thanks! This should be ready to merge.

knrafto commented 8 years ago

Sorry, thought I merged this already. My bad