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.

Sign up for GitHub

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

Jump to bottom

[#6] Add a switch for allowing comments #16

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

[#6] Add a switch for allowing comments #16

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9f49d2f
[#6] Add a switch for allowing comments
nalkuatov Jun 10, 2022
4b01213
fixup! [#6] Add a switch for allowing comments
nalkuatov Jun 13, 2022
3ed16dd
fixup! [#6] Add a switch for allowing comments
nalkuatov Jun 15, 2022
f2d05ab
fixup! [#6] Add a switch for allowing comments
nalkuatov Jun 15, 2022
2e2697f
fixup! [#6] Add a switch for allowing comments
nalkuatov Jun 16, 2022
6f9f4cc
fixup! [#6] Add a switch for allowing comments
nalkuatov Jun 27, 2022
26aedc6
fixup! [#6] Add a switch for allowing comments
nalkuatov Jun 29, 2022
fc4c8d2
fixup! fixup! [#6] Add a switch for allowing comments
nalkuatov Jun 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions core/src/Text/Interpolation/Nyan/Core.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ module Text.Interpolation.Nyan.Core
, recommendedDefaultSwitchesOptions
-- ** Field accessors for default switches options
, defSpacesTrimming
, defCommenting
, defIndentationStripping
, defLeadingNewlineStripping
, defTrailingSpacesStripping
Expand Down
4 changes: 4 additions & 0 deletions core/src/Text/Interpolation/Nyan/Core/Internal/Base.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ data PreviewLevel
-- | All switches options.
data SwitchesOptions = SwitchesOptions
{ spacesTrimming :: Bool
, commenting :: Bool
, indentationStripping :: Bool
, leadingNewlineStripping :: Bool
, trailingSpacesStripping :: Bool
Expand All @@ -100,6 +101,7 @@ data SwitchesOptions = SwitchesOptions
-- mandatory for specifying in the interpolator.
data DefaultSwitchesOptions = DefaultSwitchesOptions
{ defSpacesTrimming :: Maybe Bool
, defCommenting :: Maybe Bool
, defIndentationStripping :: Maybe Bool
, defLeadingNewlineStripping :: Maybe Bool
, defTrailingSpacesStripping :: Maybe Bool
Expand All @@ -117,6 +119,7 @@ data DefaultSwitchesOptions = DefaultSwitchesOptions
basicDefaultSwitchesOptions :: DefaultSwitchesOptions
basicDefaultSwitchesOptions = DefaultSwitchesOptions
{ defSpacesTrimming = Just False
, defCommenting = Just False
, defIndentationStripping = Just False
, defLeadingNewlineStripping = Just False
, defTrailingSpacesStripping = Just False
Expand All @@ -130,6 +133,7 @@ basicDefaultSwitchesOptions = DefaultSwitchesOptions
recommendedDefaultSwitchesOptions :: DefaultSwitchesOptions
recommendedDefaultSwitchesOptions = DefaultSwitchesOptions
{ defSpacesTrimming = Just False
, defCommenting = Just False
, defIndentationStripping = Just True
, defLeadingNewlineStripping = Just True
, defTrailingSpacesStripping = Just True
Expand Down
43 changes: 37 additions & 6 deletions core/src/Text/Interpolation/Nyan/Core/Internal/Parser.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@ import qualified Data.Text as T
import Fmt (Builder, build, fmt)
import Text.Interpolation.Nyan.Core.Internal.Base
import Text.Megaparsec (Parsec, customFailure, eof, errorBundlePretty, label, lookAhead, parse,
single, takeWhile1P, takeWhileP)
single, takeWhile1P, takeWhileP, try)
import Text.Megaparsec.Char (spaceChar)
import Text.Megaparsec.Char.Lexer (skipBlockComment, skipLineComment)
import Text.Megaparsec.Error (ShowErrorComponent (..))

newtype OptionChanged = OptionChanged Bool
Expand All @@ -25,6 +27,7 @@ newtype OptionChanged = OptionChanged Bool
-- | An accumulator for switch options during parsing.
data SwitchesOptionsBuilder = SwitchesOptionsBuilder
{ spacesTrimmingB :: (OptionChanged, Maybe Bool)
, commentingB :: (OptionChanged, Maybe Bool)
, indentationStrippingB :: (OptionChanged, Maybe Bool)
, leadingNewlineStrippingB :: (OptionChanged, Maybe Bool)
, trailingSpacesStrippingB :: (OptionChanged, Maybe Bool)
Expand All @@ -38,6 +41,7 @@ toSwitchesOptionsBuilder :: DefaultSwitchesOptions -> SwitchesOptionsBuilder
toSwitchesOptionsBuilder DefaultSwitchesOptions{..} =
SwitchesOptionsBuilder
{ spacesTrimmingB = (OptionChanged False, defSpacesTrimming)
, commentingB = (OptionChanged False, defCommenting)
, indentationStrippingB = (OptionChanged False, defIndentationStripping)
, leadingNewlineStrippingB = (OptionChanged False, defLeadingNewlineStripping)
, trailingSpacesStrippingB = (OptionChanged False, defTrailingSpacesStripping)
Expand All @@ -50,6 +54,7 @@ toSwitchesOptionsBuilder DefaultSwitchesOptions{..} =
finalizeSwitchesOptions :: MonadFail m => SwitchesOptionsBuilder -> m SwitchesOptions
finalizeSwitchesOptions SwitchesOptionsBuilder{..} = do
spacesTrimming <- fromOptional "spaces trimming" spacesTrimmingB
commenting <- fromOptional "comments handling" commentingB
indentationStripping <- fromOptional "indentation stripping" indentationStrippingB
leadingNewlineStripping <- fromOptional "leading newline stripping" leadingNewlineStrippingB
trailingSpacesStripping <- fromOptional "trailing spaces stripping" trailingSpacesStrippingB
Expand All @@ -73,6 +78,12 @@ setIfNew desc new (OptionChanged ch, old)
| old == Just new = fail $ "Switch option `" <> desc <> "` is set redundantly"
| otherwise = return (OptionChanged True, Just new)

setCommenting :: SwitchesOptionsSetter m => Bool -> m ()
setCommenting enable = do
opts <- get
res <- setIfNew "comments handling" enable (commentingB opts)
put opts{ commentingB = res }

setSpacesTrimming :: SwitchesOptionsSetter m => Bool -> m ()
setSpacesTrimming enable = do
opts <- get
Expand Down Expand Up @@ -150,6 +161,11 @@ switchesSectionP defSOpts =
, single 'S' $> False
] >>= setSpacesTrimming

, asum
[ single 'c' $> True
, single 'C' $> False
] >>= setCommenting

, asum
[ single 'd' $> True
, single 'D' $> False
Expand Down Expand Up @@ -201,6 +217,7 @@ switchesHelpMessage sopts =
(error "")
(error "")
(error "")
(error "")
-- ↑ Note: If you edit this, you may also need to update
-- the help messages below.
in mconcat
Expand All @@ -210,6 +227,11 @@ switchesHelpMessage sopts =
, ("S", "disable spaces trimming", Just False)
]

, helpOnOptions (defCommenting sopts)
[ ("c", "enable commenting", Just True)
, ("C", "disable commenting", Just False)
]

, helpOnOptions (defIndentationStripping sopts)
[ ("d", "enable indentation stripping", Just True)
, ("D", "disable indentation stripping", Just False)
Expand Down Expand Up @@ -254,11 +276,15 @@ switchesHelpMessage sopts =
, val /= defVal
]

intPieceP :: Ord e => Parsec e Text [ParsedIntPiece]
intPieceP = asum
[
intPieceP :: Ord e => SwitchesOptions -> Parsec e Text [ParsedIntPiece]
intPieceP SwitchesOptions{..} = asum [

-- ignore comments if 'commenting' switch is on
guard commenting *>
asum [ skipLineComment "--", skipBlockComment' ] $> []

-- consume normal text
one . PipString <$> takeWhile1P Nothing (notAnyOf [(== '\\'), (== '#'), isSpace])
, one . PipString <$> takeWhile1P Nothing (notAnyOf [(== '\\'), (== '#'), isSpace])

-- potentially interpolator case
, single '#' *> do
Expand Down Expand Up @@ -304,6 +330,11 @@ intPieceP = asum

]
where
skipBlockComment' = asum
[ skipBlockComment "{-" "-}"
, try $ spaceChar *> skipBlockComment "{-" "-}"
]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather write the clause with space removal as an extension of the "fast spacing" section above - to avoid extra rollbacks.

Generally, I find this approach good, you got the code grouped by the logic it is related to. But in our case performance matters (as we write a library), and the grammar is quite small, so we can afford some complication.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But in our case performance matters

Yup, totally agree


newline = PipNewline . mconcat <$> sequence
[ maybe "" T.singleton <$> optional (single '\r')
, T.singleton <$> single '\n'
Expand Down Expand Up @@ -335,7 +366,7 @@ intStringP
intStringP sopts = do
switches <- switchesSectionP sopts
_ <- single '|'
pieces <- glueParsedStrings . concat <$> many intPieceP
pieces <- glueParsedStrings . concat <$> many (intPieceP switches)
eof
return (switches, pieces)

Expand Down
2 changes: 2 additions & 0 deletions core/tests/Test/Customization.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,12 @@ _AllFieldsAreExported =
(error "")
(error "")
(error "")
(error "")
-- ↑ if you change this, also add a field to the record below
in basicDefaultSwitchesOptions
{ defIndentationStripping = Nothing
, defSpacesTrimming = Nothing
, defCommenting = Nothing
, defLeadingNewlineStripping = Nothing
, defTrailingSpacesStripping = Nothing
, defReducedNewlines = Nothing
Expand Down
39 changes: 39 additions & 0 deletions core/tests/Test/Interpolator.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,6 +297,45 @@ test_DefaultInterpolator = testGroup "Default interpolator"

]

----------------------------------
, testGroup "Commenting"

[ testCase "Basic comments" do
[int|tc|Abc -- this is a comment|]
@?= "Abc "

, testCase "Comments in arbitrary lines" do
[int|tc| -- comments at the beginning
My text -- comments in the middle
-- comments at the end
|] @?= " \nMy text \n\n"

, testCase "Inline block comments" do
[int|tc| My {- inline comment -} text
Martoon-00 marked this conversation as resolved.
Show resolved Hide resolved
|] @?= " My text\n"

, testCase "Multiline block comments" do
[int|tc| My text {- multiline block
comments are fun,
aren't they?
-}
|] @?= " My text\n"

, testCase "Line comments do not affect the indentation" do
[int|tc|
The beginning
-- some comments in the middle
The end
|] @?= " The beginning\n\n The end\n"

, testCase "Block comments do not affect the indentation" do
[int|tc|
The beginning {- some clarifying
comments in the middle -}
The end
|] @?= "The beginning\nThe end\n"
]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also like tests on:

  1. Unusual space - e.g. tab;
  2. On several spaces before comment.

Behaviour in such cases should be fixed.

And documented (no need to mention edge cases, just write the rule in one-two sentences so that it is unambiguous in regard to these edge cases).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Behaviour in such cases should be fixed.

"My text (4 spaces) { - comments -} " -> do you mean that, in this case, the result should be "My text "? Do several spaces should get truncated to a single one?

And, aside from that, please consider this one:

{int|tc|
      My text
  -- comments
      end
|]

results in " My text\n\n end" - the indentation becomes two spaces, since the line comment has two leading empty spaces. Intuitively, one might expect the indentation to be totally stripped, and since we now support extra spaces removal for block comments, shouldn't we handle this case too?


]

]
Expand Down
2 changes: 1 addition & 1 deletion core/tests/Test/Parser.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ test_TextParser = testGroup "Main text parser"

basicSwitchesOptions :: SwitchesOptions
basicSwitchesOptions =
SwitchesOptions False False False False AnyFromBuilder False False PreviewNone
SwitchesOptions False False False False False AnyFromBuilder False False PreviewNone

test_SwitchesParser :: TestTree
test_SwitchesParser = testGroup "Switches parser"
Expand Down
24 changes: 24 additions & 0 deletions full/src/Text/Interpolation/Nyan/Tutorial.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,20 @@ In case monadic actions have side effects, they will be applied in the same orde
in which placeholders appear in the quoter. /But you are not going to use this
behaviour, don't you?/

==== c ([c]omments handling)

Handle line comments starting with @--@ and/or block comments enclosed in @{- ... -}@.

>>> :{
[int|c|My text -- this is a line comment|]
:}
"My text "

>>> :{
[int|c|My {- this is a block comment -} text|]
:}
"My text"

==== t (return [t]ext)

The quoter will return concrete t'Text'.
Expand Down Expand Up @@ -272,6 +286,16 @@ affects indentation stripping to also ignore the line with @|]@:
:}
"\nValue 1 is 5, value 2 is 10\n"

* Comments remain invisible for other switches and thus do not affect them.
Consider the following indentation stripping example:

>>> :{
[int|c|
The beginning
-- some comments in the middle
The end|]
:}
"The beginning\n\nThe end"

=== Customizing the interpolator

Expand Down