@tommai78101 https://stackoverflow.com/questions/10842131/xml-comments-and

I don't know XML, but I'm guessing this is intentional.

Yeah, I knew about the Stack Overflow, and how it's already in the Opinion section in OP, but I don't know about the last part where leaving out the 'end' parameter is intentional.

I worked on PR #87 that caused this behavior, it was intentional at the time, but in hindsight I agree with @tommai78101's opinion. However, last time @Ingramz's opinion was that "From the point where the error is first registered, the rest of the document will be invalid." So perhaps we should come to an agreement on what is the best way to do this before I open another PR.

The end part is left out intentionally so that it could not break out of the rule anymore. In theory this should make it easier to locate the source of error.

If there is a good reason why it should be changed or a reasonable use case that makes the developer experience more comfortable, we can always make the necessary adjustments.

I've created #101 to address this issue and also #96

I understand the comment above from @Ingramz and the intention to increase visibility on invalid comment is good but current approach blocks highlighting of other parts that are valid or detection of other issues further in the document.

Similar to compilers and other grammars, attempting to recover from error and continue parsing translates to better developer experience other than going though all possible issues one by one.

A possible midway approach could be to tokeninze with invalid.illegal.bad-comments-or-CDATA.xml but only until the next --> is found.

My understanding of the problem we are trying to solve now is that people get frustrated when they accidentally leave or type in -- which ruins the highlighting for the rest of the file, so it becomes impossible to work on until that segment is eliminated. If this is the case, we should mention this explicitly before committing to a change. Otherwise we are just blindly catering to someones opinion, which is frustrating if someone else comes with a different opinion later on. Please do correct me if I am mistaken.

Now that we have something to solve, I think we should go with the midway approach such that erroneous parts shall not be attempted to highlight correctly at all as proposed by @vfcp. Terminating the comment at the next valid --> seems like a reasonable tradeoff to me. Also consider cases containing --->, in which case I think it is important that we do not stop the comment there as it is cannot be part of a valid comment either.

If someone else has any thoughts on this, feel free to provide your take on this.

So, for the midway approach, I'm proposing adding an end regexp like this (^|[^-])(?=-->)

Final rule would be:

      {
        'begin': '<!--'
        'captures':
          '0':
            'name': 'punctuation.definition.comment.xml'
        'end': '-->'
        'name': 'comment.block.xml'
        'patterns': [
          {
            'begin': '--(?!>)'
            'end': '(^|[^-])(?=-->)'
            'captures':
              '0':
                'name': 'invalid.illegal.bad-comments-or-CDATA.xml'
          }
        ]
      }

I'll test this in linguist, vscode's textmate and atom's firstmate before updating my PR.

@vfcp in general it looks good. The captures section in the subpattern probably doesn't do what you want, move the name to the same level as begin / end instead. You'll probably notice it when you test it yourself.

While investigating this, I came up with a slightly different approach to capture the comments:

      {
        'begin': '<!--'
        'captures':
          '0':
            'name': 'punctuation.definition.comment.xml'
        'end': '(?<!-)-->'
        'name': 'comment.block.xml'
        'patterns': [
          {
            'begin': '--'
            'end': '(?=-->)'
            'name': 'invalid.illegal.bad-comments-or-CDATA.xml'
          }
        ]
      }

It should be doing the same thing, but it feels a little cleaner to me. Let me know what you think.

@Ingramz Your solution works much better with Linguist highlighter and it's also much easier to understand and maintain.

The ---> special case is handled on the correct level and the invalid comment begin/end rules reflect exactly the behavior we are trying to achieve.

I've updated the PR #101 code and tests accordingly.

Thanks a lot for your feedback!