New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for HCL #1594

Merged
merged 10 commits into from Dec 27, 2018

Conversation

Projects
None yet
4 participants
@outsideris
Copy link
Contributor

outsideris commented Oct 24, 2018

@RunDevelopment
Copy link
Collaborator

RunDevelopment left a comment

Thank you for creating this language definition!

There are, however, a few things we have to fix before we can merge this. Apart from the comments, I left you, these things have to be addressed:

  1. Please don't use capturing groups when you don't need to. This is confusing because I expect the captured text to be used in some way. So please use non-capturing groups.
  2. There are a few cases where you have lists of tokens with only one pattern ('abc': [ { /* stuff */ } ]). Just assign the pattern to the token ('abc': { /* stuff */ }).
    Same goes for pattern objects of the form: { pattern: /regex/ }. You can always just inline /regex/.
  3. Multiline strings are not supported.
  4. Please add a punctuation token for the brackets of objects and arrays.

After that is done, I will continue to review your PR.

Also, I'm not very familiar with HCL so please tell me if I made a mistake somewhere.

Show resolved Hide resolved components/prism-hcl.js Outdated
Show resolved Hide resolved components/prism-hcl.js Outdated
Show resolved Hide resolved components/prism-hcl.js Outdated
Show resolved Hide resolved components/prism-hcl.js Outdated
Show resolved Hide resolved components/prism-hcl.js Outdated
Show resolved Hide resolved components/prism-hcl.js Outdated
Show resolved Hide resolved components/prism-hcl.js Outdated
Show resolved Hide resolved components/prism-hcl.js Outdated
Show resolved Hide resolved components/prism-hcl.js Outdated
Show resolved Hide resolved tests/languages/hcl/keyword_feature.test Outdated
@outsideris

This comment has been minimized.

Copy link
Contributor

outsideris commented Nov 4, 2018

Sorry for late, I will fix them until next weekend.

@robmorgan

This comment has been minimized.

Copy link

robmorgan commented Nov 19, 2018

@outsideris any luck? need a hand?

@outsideris

This comment has been minimized.

Copy link
Contributor

outsideris commented Nov 19, 2018

I started to work on it.
Sorry, I really was busy for company things.

@outsideris

This comment has been minimized.

Copy link
Contributor

outsideris commented Nov 19, 2018

I'm updating code as your great feedback. Thank you!!
I will push new commits soon after I'm done. Sorry for late response.

@robmorgan

This comment has been minimized.

Copy link

robmorgan commented Nov 19, 2018

@outsideris no need to apologise for open source work mate ;)

@outsideris outsideris force-pushed the outsideris:support-hcl branch from f9013cb to 6ff5cf9 Nov 20, 2018

@outsideris

This comment has been minimized.

Copy link
Contributor

outsideris commented Nov 20, 2018

I modified them as your feedback.

@RunDevelopment

This comment has been minimized.

Copy link
Collaborator

RunDevelopment commented Nov 29, 2018

@outsideris I'm sorry, I missed your last commit! I will review the changes ASAP.

@maartenvanderhoef

This comment has been minimized.

Copy link

maartenvanderhoef commented Dec 3, 2018

@outsideris Will this also parse hcl2 ? If not, would it be an idea to name this hcl1 ?

@outsideris

This comment has been minimized.

Copy link
Contributor

outsideris commented Dec 4, 2018

@maartenvanderhoef It is not parse hcl2 and hcl2 is still experimental.
For me, name "hcl" is fine because HashiCorp also call them as "HCL" and "HCL2". How do you think?

@outsideris outsideris force-pushed the outsideris:support-hcl branch from 6ff5cf9 to e1fd27d Dec 4, 2018

@outsideris

This comment has been minimized.

Copy link
Contributor

outsideris commented Dec 4, 2018

I rebased onto latest master.

@maartenvanderhoef

This comment has been minimized.

Copy link

maartenvanderhoef commented Dec 4, 2018

hi @outsideris, you're right, let's stick Hashicorp's own naming.

@RunDevelopment
Copy link
Collaborator

RunDevelopment left a comment

Sorry for the delay!

I have a few problems beside my comments:

  1. Indentation.
  2. The use of capturing groups when the captured content is not used.
    You use capturing groups a lot and they are necessary for lookbehinds and backreferences but all groups which are not used in that way should be non-capturing.
    This is to make the patterns easier to understand because if there is a capturing group, you'll expect the captured text to be used.
  3. Unnecessary surrounding array/objects.
    In a few places, there is an array wrapped around a single object (e.g. 'type': [ { /* pattern and so on */ } ] in which case you can replace the array with its content.
    In other places, there are objects which only contain the pattern property. Those can be replaced with the pattern itself.
    Again to make it easier to read and understand.
  4. Instead of [\w-]+|"[\w-]+" you should use [\w-]+|"(?:[^\n\r\\"]|\\.)*" to captur things like "abc\"def": 8.

We will talk about interpolation, greedy strings and the keyword patterns once this is done.
Again, I'm really sorry for the delay.

Show resolved Hide resolved components/prism-hcl.js Outdated
Show resolved Hide resolved components/prism-hcl.js Outdated
Show resolved Hide resolved components/prism-hcl.js Outdated
Show resolved Hide resolved components/prism-hcl.js Outdated
Show resolved Hide resolved components/prism-hcl.js Outdated
Show resolved Hide resolved components/prism-hcl.js Outdated
Show resolved Hide resolved components/prism-hcl.js Outdated
Show resolved Hide resolved components/prism-hcl.js Outdated

@outsideris outsideris force-pushed the outsideris:support-hcl branch from e1fd27d to 40c4578 Dec 16, 2018

@outsideris

This comment has been minimized.

Copy link
Contributor

outsideris commented Dec 16, 2018

I fixed as your feedback. Sorry for bordering you because I'm not regexp expert.

@RunDevelopment
Copy link
Collaborator

RunDevelopment left a comment

A few more things and then we can talk about interpolation.

Also, please replace all arrays which only contain one item with said item. A good example of this is interpolation.

(And don't worry. That doesn't borther me at all. Nobody is a regex master from the get go.)

Show resolved Hide resolved components/prism-hcl.js Outdated
Show resolved Hide resolved components/prism-hcl.js Outdated
Show resolved Hide resolved components/prism-hcl.js Outdated
Show resolved Hide resolved components/prism-hcl.js Outdated
Show resolved Hide resolved components/prism-hcl.js Outdated
Show resolved Hide resolved examples/prism-hcl.html Outdated
Show resolved Hide resolved tests/languages/hcl/string_feature.test Outdated
@outsideris

This comment has been minimized.

Copy link
Contributor

outsideris commented Dec 17, 2018

Also, please replace all arrays which only contain one item with said item.

I believe I didn't understand what you mean exactly. I can't find more arrays which only contain one item.

@RunDevelopment

This comment has been minimized.

Copy link
Collaborator

RunDevelopment commented Dec 17, 2018

Maybe another example: hcl.keyword.inside.type is currently defined as follows:

'type': [ // start of array
	{	// one item
		pattern: /(resource|data|\s+)(?:[\w-]+|"[\w-]+")/i,
		lookbehind: true,
		alias: 'variable'
	}
] // end of array

But this can be replaced with a simpler expression:

'type': {
	pattern: /(resource|data|\s+)(?:[\w-]+|"[\w-]+")/i,
	lookbehind: true,
	alias: 'variable'
}

Other examples like this can be found on line 7, 19, 32, 36, and 48.

Show resolved Hide resolved components/prism-hcl.js Outdated

@outsideris outsideris force-pushed the outsideris:support-hcl branch from 5406c69 to 890be15 Dec 18, 2018

@outsideris

This comment has been minimized.

Copy link
Contributor

outsideris commented Dec 18, 2018

fixed.

@RunDevelopment

This comment has been minimized.

Copy link
Collaborator

RunDevelopment commented Dec 20, 2018

@outsideris
Good!
The things left to do now are:

  1. Interpolation as discussed here.
  2. Here-doc strings.

outsideris added some commits Oct 24, 2018

Add support for HCL
Signed-off-by: Outsider <outsideris@gmail.com>
apply code reviews
Signed-off-by: Outsider <outsideris@gmail.com>

outsideris added some commits Dec 17, 2018

apply code reviews
Signed-off-by: Outsider <outsideris@gmail.com>
remove one item array and fix capturing group unused
Signed-off-by: Outsider <outsideris@gmail.com>
move interpolation into string
Signed-off-by: Outsider <outsideris@gmail.com>
support heredoc
Signed-off-by: Outsider <outsideris@gmail.com>

@outsideris outsideris force-pushed the outsideris:support-hcl branch from 890be15 to c05d498 Dec 25, 2018

@outsideris

This comment has been minimized.

Copy link
Contributor

outsideris commented Dec 25, 2018

@RunDevelopment
I moved interpolation into string as your advice. Thanks. -> 8022bed
I supported heredoc. -> c05d498

@RunDevelopment
Copy link
Collaborator

RunDevelopment left a comment

Good work!

I left you a few comments and after that is done, I think we are ready to merge this.

Show resolved Hide resolved components/prism-hcl.js Outdated
Show resolved Hide resolved components/prism-hcl.js
Show resolved Hide resolved components/prism-hcl.js Outdated
Show resolved Hide resolved components/prism-hcl.js Outdated
Show resolved Hide resolved components/prism-hcl.js Outdated
Show resolved Hide resolved components/prism-hcl.js
Show resolved Hide resolved components/prism-hcl.js Outdated
@outsideris
Copy link
Contributor

outsideris left a comment

I changed them as your feedback.

apply code reviews
Signed-off-by: Outsider <outsideris@gmail.com>
@RunDevelopment
Copy link
Collaborator

RunDevelopment left a comment

Looks good!

Two small things:

  1. Please make comment the first pattern and heredoc greedy. This will solve the problem of false comments inside heredoc strings.
  2. Please replace all occurrences of "?[\w-]+"? and [\w-]+|"[\w-]+" with (?:[\w-]+|"(?:\\[\s\S]|[^\\"])*"). All of these occurrences are in keyword.
apply code reviews
Signed-off-by: Outsider <outsideris@gmail.com>
@outsideris

This comment has been minimized.

Copy link
Contributor

outsideris commented Dec 26, 2018

Fixed! :)

Show resolved Hide resolved components/prism-hcl.js Outdated
Show resolved Hide resolved components/prism-hcl.js Outdated
apply code reviews
Signed-off-by: Outsider <outsideris@gmail.com>
@outsideris

This comment has been minimized.

Copy link
Contributor

outsideris commented Dec 27, 2018

Done.

fix number patterns
Signed-off-by: Outsider <outsideris@gmail.com>
@outsideris

This comment has been minimized.

Copy link
Contributor

outsideris commented Dec 27, 2018

@RunDevelopment Fixed. Sorry for I missed it.

@RunDevelopment RunDevelopment merged commit c939df8 into PrismJS:master Dec 27, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@outsideris

This comment has been minimized.

Copy link
Contributor

outsideris commented Dec 27, 2018

@RunDevelopment You are awesome. I couldn't make this PR without you. Thank you.

@robmorgan

This comment has been minimized.

Copy link

robmorgan commented Dec 30, 2018

Good job! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment