New pattern for yamlMapping breaks ansibleString

[benjifisher]

Commit 8050b3fb42a35ff66b0616c8d85653696ed686b5 breaks this silly example:

- name: silly rule
  command: echo "when {{ when }}"
  when: some_condition

The problem is that the first } now ends the yamlMapping group, and so the second " starts an ansibleString group, which continues onto the last line. Here is a screenshot:

[chase]

Hopefully I'll have a fix worked out by the time my bus reaches my destination. Feel free to patch things up before me, though. No idea when I'll have reliable WiFi again.

[chase]

Turns out my first solution seems to work, just adding skip for yamlMapping with }} works great. I'll push the fix and some code cleanups later tonight.

[benjifisher]

It occurs to me that I am not sure whether to add comments to the issue or the related PR.

I tried the following simple test:

- name: "test case for #26 {}"

The } character ends the yamlMapping in both master and issue_26 branches. When I check out 5a31633, I do not have this problem.

I do not think there is a quick fix, as I said in PR #25. I suggest you revert 8050b3f.

[chase]

I suppose that comments on a PR should cover issues with the committed code; however, comments on issues should discuss the issue's content directly.

[chase]

I feel like reverting right now is just going to be trading this issue for issue #16. We could revert, close this issue, and reopen #16 if you feel that 8050b3f is more damaging than #16.

Personally I don't think that the fix is actually too complicated, unlike the scalar block issue. I feel that with a little more coaxing we'll have this ready to go in a timely manner.

[benjifisher]

Yes, that is what I am suggesting: revert the commit and reopen #16. We disagree about how hard this is to get right. I think that recognizing YAML flow and block structures correctly will require adding several new syntax groups, and we should do this on a development branch.

Vim syntax highlighting really is pretty arcane.

[chase]

So here's what I'm weighing my opinion on... Reverting 8050b3f to goes back to breaking:

• Flow mappings
• Inline comments
• Scalar values

But it allows:

• The use of { and } within plain mapping values
• The use of { and } within Ansible argument styled quoted mapping values

Merging #27 fixes what reverting to before 8050b3f breaks, but it breaks what seems to be a small edge case for values.

I feel that so long as we have these edge cases documented and tracked as an issue, merging #27 is a better short term solution. After all, we can always revert to before 8050b3f after the merge in a development branch to work out the issues in #16 and #27 completely.

[chase]

In the long run, you are the maintainer, so it's your call. Feel free to do what you decide is best :smiley:

[chase]

Basically, here is what I propose:

1. Merge #27 into master so that the only bugs are the edge cases mentioned above
2. Checkout a new branch based at 5a31633d93f1663831c49ac6baec8b1ecf7d536b called flow_mapping
3. In the flow_mapping branch, complete the development work you mentioned about creating appropriate syntax groups, regions, and matches until you have a feature complete and bug free solution
4. Revert master to 5a31633d93f1663831c49ac6baec8b1ecf7d536b
5. Merge the flow_mapping branch

This way we get a slightly more stable and feature complete master than we currently have and you get to implement things the correct and accurate way in the end.

[benjifisher]

See my comment on #27: it causes a regression on #11, so the current version of that branch is not a viable option.

Looking at your screenshots in #27, Line 4 has a lot of problems created by 8050b3f.

Also, 8050b3f highlights some things as comments that should not be. When I run this task

  - name: "Test the use of {brace} characters. # Comment?"
    debug:
      msg: "Print these {brace} characters. # Comment?"

I get

TASK: [Test the use of {brace} characters. # Comment?] ************************ 
ok: [localhost] => {
    "msg": "Print these {brace} characters. # Comment?"
}

(If I leave out the quotation marks, then it actually is a comment.)

So it is not clear to me that 8050b3f does more good than harm. I prefer things like 5a31633 that make a little progress without breaking anything else. If it is really up to me, I think I will revert 8050b3f, but I will sleep on it first.

[chase]

It really is up to you, while this may have been my creation originally, it greatly reflects what the community has requested of it. At least in my eyes, as the maintainer you are ultimately in charge of what does or does not make the cut. These days I am merely a developer, so if it doesn't hold to your standards, I can only try to sway your opinion on whatever code I throw your way.

As it stands, yes, 8050b3f is bad news. It was a poor decision on my behalf to push straight to master. My judgement on that "quick fix" has led to all this fallout. :yum:

As for the errors in line 4 of 8050b3f, which is prior to the #27 fiasco, I don't actually notice anything other than the fact that the first word is still being highlighted as a plain scalar. Could expand on that a little bit so I'm on the same line of thought as you for later implementations?

As for 8050b3f's handling of comments, this was something else I thought I fixed in #27. What triggers your specific case is } closes a non-existent flow mapping match inside of the string. Although #27 causes a performance regression for you, so it's being dropped anyway. I'd like to keep the issue_26 branch from #27 there for reference purposes for future solutions.

[chase]

Although a completely different editor, I feel this xkcd reflects things pretty well:

[benjifisher]

On LIne 4 of your screenshot for the 8050b3f commit, it looks to me as though

• The keys name and version get different highlighting.
• The two closing }} get different highlighting.

Maybe intentional, and not a change, but the two parts of item.name get different highlighting.

[chase]

Crazy how I only noticed once you brought it up. Great having another pair of eyes :+1:

[chase]

The two parts of item.name are intentional, for what it is worth. That is a feature of the Jinja syntax.