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

nalkuatov · 2022-06-10T21:15:35Z

This implements a switch to allow having comments

Description

Related issue(s)

Resolves #6

✅ Checklist for your Pull Request

Related changes (conditional)

Tests
- If I added new functionality, I added tests covering it.
- If I fixed a bug, I added a regression test to prevent the bug from
  silently reappearing again.
Documentation
- I checked whether I should update the docs and did so if necessary:
  - README
  - Design decisions
  - Haddock
Public contracts
- Any modifications of public contracts comply with the Evolution
  of Public Contracts policy.
- I added an entry to the changelog if my changes are visible to the users
  and
- provided a migration guide for breaking changes if possible

Stylistic guide (mandatory)

My commits comply with the policy used in Serokell.
My code complies with the style guide.

Martoon-00 · 2022-06-14T10:30:26Z

Good, I see you are delving into the project quite well.

One thing to note: to me it seems like we could handle comments skipping at parsing stage directly, this should be simpler. It is fine to pass commenting switch to intPieceP parser, it just happened that no switches managed parser behaviour till now.

core/src/Text/Interpolation/Nyan/Core/Internal/Parser.hs

Martoon-00 · 2022-06-15T16:29:38Z

You probably was thinking about whether to add this too: I would also skip {- ... -} comments, just as in Haskell.

Martoon-00 · 2022-06-22T16:45:59Z

Functionally this looks great 👍

Now you also need to update documentation. I would include the new switch description in Tutorial.hs into the list of switches.

Plus, in Switches interaction it would be nice to mention that comments are not visible by other switches, e.g.

txt = [int||
  My text
// comment
  other text
  |]

would have ... result. And if we guarantee that in docs, we will also need a test on such case.

core/src/Text/Interpolation/Nyan/Core/Internal/Parser.hs

Martoon-00 · 2022-06-28T20:25:58Z

full/src/Text/Interpolation/Nyan/Tutorial.hs

@@ -223,6 +223,10 @@ The quoter will return concrete t'Builder'.

 The quoter will return any type with t'FromBuilder' instance.

+==== c ([c]omments handling)
+
+Ignore line comments starting with @--@ and/or block comments enclosed in @{- ... -}@.


I would use some different wording, as "ignore" may mean "ignore it in processing, do not treat it in a special way" and so "leave it intact".

Sure, I've decided to go for Handle comments and providing some clarifying examples. Hope it makes more sense now :)

BTW I've moved the lines upwards a little, now the section's before the ones about the return types.

Oh that's actually good that you moved this section above, right.

full/src/Text/Interpolation/Nyan/Tutorial.hs

Martoon-00 · 2022-06-29T22:06:33Z

We are approaching the perfect solution, should be mostly it.

One probably last concern that I have: the examples you provided in documentation make it fully evident that currently block comments handling has inconvenient spacing. Writing My {- comment -} text produces two spaces in between the text, and getting one space requires putting text glued with the comment from one side.

Let's make block comments ignore the space before them, if any? "Before" to make trailing block comments handled well too.

Problem: We want the interpolator to support commentaries in arbitrary lines. Solution: Implement a switch to allow writing comments and add some tests against the implementation.

nalkuatov · 2022-06-30T11:44:38Z

In this case, as far as I can see the comment does not affect the indentation just because it is already aligned with the other text, comments enabling would not change the result in this regard?

Oh, right. That test actually didn't contain the evidence, sorry

core/tests/Test/Interpolator.hs

Martoon-00 · 2022-06-30T22:58:14Z

core/src/Text/Interpolation/Nyan/Core/Internal/Parser.hs

+    skipBlockComment' = asum
+      [ skipBlockComment "{-" "-}"
+      , try $ spaceChar *> skipBlockComment "{-" "-}"
+      ]


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.

But in our case performance matters

Yup, totally agree

Martoon-00 · 2022-06-30T23:01:04Z

core/tests/Test/Interpolator.hs

+           comments in the middle -}
+              The end
+          |] @?= "The beginning\nThe end\n"
+       ]


I would also like tests on:

Unusual space - e.g. tab;

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).

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?

nalkuatov requested a review from Martoon-00 June 10, 2022 21:15

nalkuatov force-pushed the nalkuatov/#6-allow-comments branch from 262a1c4 to 9958b29 Compare June 13, 2022 09:16

nalkuatov force-pushed the nalkuatov/#6-allow-comments branch from 9a1614f to 3e2d1e1 Compare June 15, 2022 12:52

nalkuatov commented Jun 15, 2022

View reviewed changes

core/src/Text/Interpolation/Nyan/Core/Internal/Parser.hs Outdated Show resolved Hide resolved

Martoon-00 reviewed Jun 22, 2022

View reviewed changes

core/src/Text/Interpolation/Nyan/Core/Internal/Parser.hs Outdated Show resolved Hide resolved

nalkuatov requested a review from Martoon-00 June 27, 2022 15:40

Martoon-00 reviewed Jun 28, 2022

View reviewed changes

nalkuatov added 8 commits June 30, 2022 17:41

[#6] Add a switch for allowing comments

9f49d2f

Problem: We want the interpolator to support commentaries in arbitrary lines. Solution: Implement a switch to allow writing comments and add some tests against the implementation.

fixup! [#6] Add a switch for allowing comments

4b01213

fixup! [#6] Add a switch for allowing comments

3ed16dd

fixup! [#6] Add a switch for allowing comments

f2d05ab

fixup! [#6] Add a switch for allowing comments

2e2697f

fixup! [#6] Add a switch for allowing comments

6f9f4cc

fixup! [#6] Add a switch for allowing comments

26aedc6

fixup! fixup! [#6] Add a switch for allowing comments

fc4c8d2

nalkuatov force-pushed the nalkuatov/#6-allow-comments branch from 4ae43ac to fc4c8d2 Compare June 30, 2022 11:42

nalkuatov commented Jun 30, 2022

View reviewed changes

core/tests/Test/Interpolator.hs Show resolved Hide resolved

nalkuatov requested a review from Martoon-00 June 30, 2022 12:02

Martoon-00 reviewed Jun 30, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

nalkuatov commented Jun 10, 2022 •

edited

Martoon-00 commented Jun 14, 2022 •

edited

Martoon-00 commented Jun 15, 2022

Martoon-00 commented Jun 22, 2022

Martoon-00 Jun 28, 2022

nalkuatov Jun 29, 2022 •

edited

Martoon-00 Jun 29, 2022

Martoon-00 commented Jun 29, 2022

nalkuatov commented Jun 30, 2022

Martoon-00 Jun 30, 2022

nalkuatov Jul 1, 2022

Martoon-00 Jun 30, 2022

nalkuatov Jul 1, 2022

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

Are you sure you want to change the base?

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

Conversation

nalkuatov commented Jun 10, 2022 • edited

Description

Related issue(s)

✅ Checklist for your Pull Request

Related changes (conditional)

Stylistic guide (mandatory)

Martoon-00 commented Jun 14, 2022 • edited

Martoon-00 commented Jun 15, 2022

Martoon-00 commented Jun 22, 2022

Martoon-00 Jun 28, 2022

Choose a reason for hiding this comment

nalkuatov Jun 29, 2022 • edited

Choose a reason for hiding this comment

Martoon-00 Jun 29, 2022

Choose a reason for hiding this comment

Martoon-00 commented Jun 29, 2022

nalkuatov commented Jun 30, 2022

Martoon-00 Jun 30, 2022

Choose a reason for hiding this comment

nalkuatov Jul 1, 2022

Choose a reason for hiding this comment

Martoon-00 Jun 30, 2022

Choose a reason for hiding this comment

nalkuatov Jul 1, 2022

Choose a reason for hiding this comment

nalkuatov commented Jun 10, 2022 •

edited

Martoon-00 commented Jun 14, 2022 •

edited

nalkuatov Jun 29, 2022 •

edited