search

knrafto / language-bash

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

Fixes #22 #24

Closed mmhat closed 4 years ago

mmhat commented 4 years ago

I did not include tests (which I should) but I'd like to reorganize the testsuite first. But I did run a few tests manually and it looks pretty good!

mmhat commented 4 years ago

Hm, ok, this was to quick. It'll work for Statements and Statements only. But I'll use the same pattern for everything between Statement and Command.

mmhat commented 4 years ago

Ok, this should do the trick.

knrafto commented 4 years ago

Instead of FlexibleInstances, what if we add a separate function in class Pretty? Something like this (not yet sure of the name):

class Pretty a where
  pretty :: a -> Doc
  prettyNoHeredocs :: a -> Doc
mmhat commented 4 years ago

Hmm I guess that would be ok, too? I'd rather keep the Pretty class similar to those found in other pretty printing packages. Just in case we'd like to move to another pretty printer package (looking at you prettyprinter).

A separate type class PrettyNoHeredoc would be another option.

mmhat commented 4 years ago

At least FlexibleInstances is not a problem since it's around since GHC 6.8.1 and considered safe. Do you want me to rewrite the pull request according to your suggestion?

knrafto commented 4 years ago

Sorry, I didn't mean FlexibleInstances is unstable or anything, I'm just wondering if there's a simpler way. I like the separate typeclass idea, it'll look slightly cleaner (prettyNoHeredoc a vs pretty (NoHeredoc a))

mmhat commented 4 years ago

Sorry, I didn't mean FlexibleInstances is unstable or anything, I'm just wondering if there's a simpler way. I like the separate typeclass idea, it'll look slightly cleaner (prettyNoHeredoc a vs pretty (NoHeredoc a))

Ok, that's fine for me. Let's go with that!

mmhat commented 4 years ago

I have not yet tested this extensively. We really should improve the test suite. While implementing this the pretty printer produced real garbage but the tests were fine like nothing happened.

mmhat commented 4 years ago

Concerning the fail of the Travis build: Which GHC versions do we support? Tests for 8.6 and 8.8 are missing and 7.10, 8.0 seems a little bit old...

Or: Should it work for versions prior to the SemigroupMonoid proposal?

knrafto commented 4 years ago

Yeah... the GHC versions haven't been updated in a while. I'll get on that

knrafto commented 4 years ago

I'll start a new top-level comment bc IMO it's too hard to see updates on the thread in the review.

Thanks a lot for looking into bash's behavior here. I definitely agree that we shouldn't follow bash's output exactly (that way lies madness).

After some soul-searching, the only condition I care about is:

  1. if there's no heredocs, the pretty-printing is (mostly) the same as it is now (so simple pipelines all on one line, do on same line as while, etc).

I originally suggested BashDoc because I didn't think PrettyNoHeredoc would work given that constraint. But I didn't know about bash's strategy, which is to insert extra newlines only if there's heredocs but otherwise try to keep things on the same line. In that case you're right, PrettyNoHeredoc would work (although it would be slightly more complicated than your original implementation, since it has to decide which layout to do based on if there's heredocs or not).

As for which of PrettyNoHeredoc or BashDoc to implement (assuming it satisfies the condition), I'll leave that up to you. I think PrettyNoHeredoc is simpler (thus more maintainable, more readable, etc) so we should prefer it, but I'd feel bad making you rewrite the PR yet again.

mmhat commented 4 years ago

Superseded by #33. Closing.