Skip to content
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
Merged

Add support for HCL #1594

merged 10 commits into from
Dec 27, 2018

Conversation

outsideris
Copy link
Contributor

Copy link
Member

@RunDevelopment RunDevelopment left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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 Show resolved Hide resolved
@outsideris
Copy link
Contributor Author

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

@robmorgan
Copy link

@outsideris any luck? need a hand?

@outsideris
Copy link
Contributor Author

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

@outsideris
Copy link
Contributor Author

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
Copy link

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

@outsideris
Copy link
Contributor Author

I modified them as your feedback.

@RunDevelopment
Copy link
Member

RunDevelopment commented Nov 29, 2018

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

@maartenvanderhoef
Copy link

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

@outsideris
Copy link
Contributor Author

@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
Copy link
Contributor Author

I rebased onto latest master.

@maartenvanderhoef
Copy link

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

Copy link
Member

@RunDevelopment RunDevelopment left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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
@outsideris
Copy link
Contributor Author

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

Copy link
Member

@RunDevelopment RunDevelopment left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

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 Show resolved Hide resolved
@outsideris
Copy link
Contributor Author

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
Copy link
Member

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.

@outsideris
Copy link
Contributor Author

fixed.

@RunDevelopment
Copy link
Member

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

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

Signed-off-by: Outsider <outsideris@gmail.com>
Signed-off-by: Outsider <outsideris@gmail.com>
Signed-off-by: Outsider <outsideris@gmail.com>
Signed-off-by: Outsider <outsideris@gmail.com>
Signed-off-by: Outsider <outsideris@gmail.com>
Signed-off-by: Outsider <outsideris@gmail.com>
@outsideris
Copy link
Contributor Author

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

Copy link
Member

@RunDevelopment RunDevelopment left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work!

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

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 Show resolved Hide resolved
Copy link
Contributor Author

@outsideris outsideris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed them as your feedback.

Signed-off-by: Outsider <outsideris@gmail.com>
Copy link
Member

@RunDevelopment RunDevelopment left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Signed-off-by: Outsider <outsideris@gmail.com>
@outsideris
Copy link
Contributor Author

Fixed! :)

components/prism-hcl.js Outdated Show resolved Hide resolved
Signed-off-by: Outsider <outsideris@gmail.com>
@outsideris
Copy link
Contributor Author

Done.

Signed-off-by: Outsider <outsideris@gmail.com>
@outsideris
Copy link
Contributor Author

@RunDevelopment Fixed. Sorry for I missed it.

@RunDevelopment RunDevelopment merged commit c939df8 into PrismJS:master Dec 27, 2018
@outsideris
Copy link
Contributor Author

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

@robmorgan
Copy link

Good job! 👍

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

Successfully merging this pull request may close these issues.

4 participants