UI: Refactors the code-mirror linting #4866
Conversation
| @@ -15,7 +15,7 @@ | |||
| {{/if}} | |||
| <label class="type-text"> | |||
| <span>Rules <a href="{{env 'CONSUL_DOCUMENTATION_URL'}}/guides/acl.html#rule-specification" rel="help noopener noreferrer" target="_blank">(HCL Format)</a></span> | |||
| {{code-editor class=(if item.error.Rules 'error') name='Rules' value=item.Rules onkeyup=(action 'change' 'Rules')}} | |||
| {{code-editor class=(if item.error.Rules 'error') name='Rules' syntax='hcl' value=item.Rules onkeyup=(action 'change' 'Rules')}} | |||
There was a problem hiding this comment.
I see we're doing a item.name.toLowerCase() == syntax.toLowerCase() on this syntax param. It caught me a bit by surprise given it wasn't an alias or a name (hcl) and I was wondering how it knew to select HCL. Maybe just worth making it an alias instead of doing that for readability?
There was a problem hiding this comment.
You mean an ember alias yeah? Do you know how that would help here? I thought an alias was to point one property to another?
The toLowerCase was just good hygiene really, I took the mode specs from the code-mirror source and the properties/'syntax names' are in uppercase, and I didn't want to change that, so I thought there would always be the slight chance of confusion as to whether to say syntax="HCL" or syntax="hcl"
Actually looking at it I could at least use === not ==
Anyway, lemme know on the alias thing, maybe I've misunderstood what you meant.
There was a problem hiding this comment.
Oh I meant https://github.com/hashicorp/consul/pull/4866/files/98acdadaf1a3597bd3e36247b6e321aa3b1826c3#diff-c79b0ce21a1201a8516d52bd57f5404bR17 one of those things. I might be really confused.
Just hygienic regardless so LGTM.
There was a problem hiding this comment.
Ahh, of course (sorry just realized what you meant now), you're not confused, I am!
This will end up going in post 1.4.0, but if you are interested in the background (more or less), here goes:
So, that JSON blob is taken directly from the codemirror modes.js file (I think its called that). That file is massive and contains a load of stuff I'll never need (it has ALL the different modes it supports). So I took the bits I need from it, but I want to leave it as close the original source as possible.
The HCL part of the blob is actually the 'ruby' one from codemirror, I just changed the name: but kept it uppercase to be consistent to the other names. So essentially HCL 'mode' is actually 'ruby' mode. I chose the syntax name for the attribute as it made sense to me, and I didn't want to use name as that means something else in HTML and mode didn't seem quite right as this is ruby mode..but hcl :S
To choose the syntax/mode, it just looks through the JSON blob, looking for 'HCL'.toLowerCase(). I could have just hardcoded the names in the JSON blob to be lowercase, but see above about wanting to change as little as possible.
|
Hey! Thanks for having a look!
It addresses a tiny bug:
So if the linting editor is to go in 1.4.0 then this should also really. John |
|
@johncowen sounds good! |
|
Just for reference, the bug fix in here has been done in a separate PR, so this one can wait for merging for the moment - I'm changed the base branch also |
Also see:
#4814 (comment)
This moves the extra code mirror linting functionality into a separate service so I'm not changing things I don't own.
I also noticed that when editing rules of legacy tokens the editor wasn't restricted to HCL only, so this adds an attribute to the component for that view to restrict the editor to HCL only.
I also considered moving the mode configuration into ember configuration, but decided against in the end as I don't want all of this configuration going in a meta tag attribute. There's still some nice work that could be done on top of this, but for now I'll at least wait until we revisit the look and feel also.