Comments multiple lines after conditional comments not handled as expected

[teng-vungle]

LAMS rule exemption block will cause parsing error if it's followed by a regular comment block.

Example that causes parsing error

view: some_view {

  # LAMS
  # rule_exemptions: {
  #   K1: "Doesn't need primary key"
  # }

  # Some other developer comment
}

Example without error (comments separated by non-comment contents):

view: some_view {

  # LAMS
  # rule_exemptions: {
  #   K1: "Doesn't need primary key"
  # }
  
  suggestions: no
  
  # Some other developer comment
}

Another example without error (regular comments come before LAMS):

view: some_view {
  # Some other developer comment

  # LAMS
  # rule_exemptions: {
  #   K1: "Doesn't need primary key"
  # }
}

What's expected:

Comment blocks—when separated by an empty line—should stop being meaningful for LAMS, and they shouldn't cause parser error because of that.

[fabio-looker]

Hi @teng-vungle - sorry I didn't see this until just now, otherwise I would have tried to squeeze the fix in with the v3 release that just went out.

Anyway, I will take a look at this behavior and double check to make sure there isn't some obscure reason why it works this way, otherwise your expected behavior makes sense to me!

[tkovich]

I found this to be the case too! A comment directly after LAMS (between LAMS and any other manifest comment) results in a LAMS error. If I double-comment it out, it works fine.

It seems like with commented-out content, LAMS doesn't know where to stop reading and trying to interpret something. It "stops" as soon as it finds any parameter in regular LookML, but anything commented out between the end of LAMS and the beginning of other content, it tries to interpret.

Fabio -- in case it's helpful, here's the error I was getting:

Run lams --reporting=save-yes
Parsing project...
Issues occurred during parsing (containing files will not be considered. See messages for details.)
Parsing done!
Checking rules...
No rules specified in manifest. As of LAMS v3, built-in rules must be opted-in to.
Rules done!
Rule Location Description
🛈 ....... project........................................ Project manifest settings read from manifest.lkml
🛈 ....... project........................................ Manifest properties: error
❌ P1..... file:manifest.lkml............................. Parsing error: Expected "#", ":", or [ \t\n\r] but "s" found.
Error: Process completed with exit code 1.

There were, in fact, lots of rules specified in the manifest, both explicit inclusions + custom rules.

The line it was trying to read for me was:

# This styling is used to wrap around the additional links buttons

When I changed that line to:

# # This styling is used to wrap around the additional links buttons
... it ran fine

And when I changed it to

# This x styling is used to wrap around the additional links buttons

it returned:
❌ P1..... file:manifest.lkml............................. Parsing error: Expected "#", ":", or [ \t\n\r] but "x" found.

So it was "skipping" the word "This" in the comment, but once it reached the "s" or "x", it threw the parsing error.

(I can't repro this now due to the new error I'm getting, but if it would be helpful, happy to hop on a call and show you Github Action history of the tests I ran when I was figuring this out! Feel free to hit me up -- [email protected])

[fabio-looker]

Thanks for the offer for more info.

I think the cause is pretty clear here, namely the \s* part of the regex on this line: https://github.com/fabio-looker/node-lookml-parser/blob/master/lib/parse/index.js#L21

Unfortunately, I had just prioritized this lower as:

1. it is unexpected behavior rather than a bug per se
2. it has a reasonable "work around"
3. it could break users who depend on the current format and may have line breaks in the middle of their conditional comment blocks

I'm still a little undecided on how I would like to release this change without disrupting people... perhaps a command-line flag, although it would need to be passed down into the parser module as well