knrafto / language-bash

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

Grab bag of minor improvements #16

Closed pbiggar closed 8 years ago

pbiggar commented 8 years ago

This is a grab bag of minor fixes that things slightly easier on me:

I can split these into multiple PRs or cut out commits that you don't like: let me know what you prefer!

pbiggar commented 8 years ago

Added one more bug fix: the logic in heredoc was wrong. Also included tests.

The logic was reversed - it's the quoted heredoc that does no expansion. Here's the spec from man bash: The format of here-documents is:

          <<[-]word
                  here-document
          delimiter

   No parameter and variable expansion,  command  substitution,  arith-
   metic expansion, or pathname expansion is performed on word.  If any
   characters in word are quoted, the delimiter is the result of  quote
   removal  on  word,  and  the  lines  in  the  here-document  are not
   expanded.  If word is unquoted, all lines of the  here-document  are
   subjected  to  parameter expansion, command substitution, and arith-
   metic expansion, the character sequence \<newline> is ignored, and \
   must be used to quote the characters \, $, and `.
knrafto commented 8 years ago

Thanks a lot for the PR! :D

I think it's pretty clear that I haven't touched this library since Stack was released, but I'll try to build it again and review the PR this weekend (or Monday).

pbiggar commented 8 years ago

Thanks! I took a look at fixing the arithmetic and it's way beyond my understanding of parsec. If you're interested in fixing that, that would be very awesome 😁. If I had to guess, it seems to look for 4 sets of parens but unsure.

knrafto commented 8 years ago

I'll take a look. It might be related to #15

knrafto commented 8 years ago

My guess though is that the parser at https://github.com/knrafto/language-bash/blob/master/src/Language/Bash/Parse/Word.hs#L157 is the culprit

knrafto commented 8 years ago

Fixed it! I can't push to your branch, but here's a patch:

From 2b34962d7ed4bb1c5e874b164da6d8cfb7e98d8f Mon Sep 17 00:00:00 2001
From: Kyle Raftogianis <knrafto@gmail.com>
Date: Sun, 25 Sep 2016 20:21:29 -0700
Subject: [PATCH] Fix parsing of arithmetic expressions (fixes #15)

---
 src/Language/Bash/Parse/Word.hs |  6 +++---
 tests/Tests.hs                  | 16 +++++++++++-----
 2 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/src/Language/Bash/Parse/Word.hs b/src/Language/Bash/Parse/Word.hs
index 111e606..9e407b0 100644
--- a/src/Language/Bash/Parse/Word.hs
+++ b/src/Language/Bash/Parse/Word.hs
@@ -154,10 +154,10 @@ backquote = Backquote <$> matchedPair '`' '`' False escape

 -- | Parse an arithmetic expression.
 arith :: Stream s m Char => ParsecT s u m String
-arith = B.toString <$> parens <?> "arithmetic expression"
+arith = B.toString <$> arithPart <?> "arithmetic expression"
   where
-    parens = B.many inner
-    inner  = B.matchedPair '(' ')' parens
+    arithPart = B.many inner
+    inner     = B.noneOf "()" <|> B.char '(' <+> arithPart <+> B.char ')'

 -- | Parse a parenthesized substitution.
 subst :: Stream s m Char => ParsecT s u m String
diff --git a/tests/Tests.hs b/tests/Tests.hs
index d5fcab9..4a2b01c 100644
--- a/tests/Tests.hs
+++ b/tests/Tests.hs
@@ -91,15 +91,21 @@ unittests = testGroup "Unit tests"
                   heredocDelim = "EOF",
                   heredocDelimQuoted = True,
                   hereDocument = expandString "asd\\`\n"}])
-
+  , tp "echo $((2 + 2))"
+       (Command
+        (SimpleCommand [] [expandString "echo", [ArithSubst "2 + 2"]])
+        [])
+  , tp "((2 + 2))"
+       (Command (Arith "2 + 2") [])
+  , tp "echo $(((2 + 2)))"
+       (Command
+        (SimpleCommand [] [expandString "echo", [ArithSubst "(2 + 2)"]])
+        [])
   ]

 failingtests :: TestTree
 failingtests = testGroup "Failing tests" (map expectFail
-  [
-    tp "echo $((2+2))"
-       (Command (Arith "2 + 2") [])
-  ])
+  [])

 tests :: TestTree
 tests = testGroup "Tests" [properties, unittests, failingtests]
-- 
2.7.4 (Apple Git-66)

The rest of the PR looks good. I'll merge and add the arith fix, and then release on Hackage.

Thanks for your help!

pbiggar commented 8 years ago

Thanks for the review and the fix!!