New pattern for yamlMapping breaks ansibleString

[benjifisher]

Commit 8050b3f 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 😃

[chase]

Basically, here is what I propose:

1. Merge Remove unused yamlScalar code and fix yamlMapping #27 into master so that the only bugs are the edge cases mentioned above
2. Checkout a new branch based at 5a31633 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 5a31633
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.