theme-tools

theme-tools copied to clipboard

What are you adding in this PR?

Adds a VariableName check, for a set naming convention to own liking, from the formats below. Supports the following formats: camelCase, PascalCase, snake_case (Default) and kebab-case

What did you learn?

The ins and outs of the theme-tool repo, and how it works.

Before you deploy

• [x] This PR includes a new checks or changes the configuration of a check
  • [x] I included a minor bump changeset
  • [x] It's in the allChecks array in src/checks/index.ts
  • [x] I ran yarn build and committed the updated configuration files

I have signed the CLA!

@karreiro Fixed the identical occurences, thanks for the suggestion on replacing the Map handler!

Thank you for the updates, @tmmgrafikr! I noticed just one last minor detail, and with that applied, I believe we're ready to merge 🚀

Ah my bad, good catch. Now removed

Thank you, @tmmgrafikr!

What's the problem here?

Documentation for this check is missing!

Documentation for this check is missing!

I've added an issue regarding this: https://github.com/Shopify/theme-tools/issues/395 I don't know how the docs site works, thought it was automated based on the contents of this repo, but guess not 😅

@tmmgrafikr Why numbers are not accepted in variable_name?

@tmmgrafikr Why numbers are not accepted in variable_name?

That one is a bug, will look into it in near future. Or a "bug", not sure, based on how snake_case works, the correct format would be first_3_d_model, because of the number.

Else you're more than welcome to add a PR fix on this, it simply uses lodash different casing formatters, as validators

@thagxt @tmmgrafikr I’ve opened an issue about numbers, in my opinion first_3d_model should be accepted as correct, as it follows a long-standing naming convention in Shopify themes:

https://github.com/Shopify/theme-tools/issues/397

I looked into customizing it however as the current implementation relies on lodash, we would need to change the RegEx that matches against unicode words, which is beyond my RegEx skills personally:

https://github.com/lodash/lodash/blob/main/src/.internal/unicodeWords.ts

Interestingly, the words function switches to using unicodeWords once it encounters a number in front of a word or a number following a word, otherwise it uses the simpler asciiWords function. If I edit words to force asciiWords, the words array returned is ['first', '3d', 'model'] and the snakeCase result does become first_3d_model. However, the return value of unicodeWords is ['first', '3', 'd', 'model'], which results in first_3_d_model.

So there’s a mismatch in results there between the two lodash functions. words can also take a custom RegEx pattern as the second argument, I wonder if the way around this is to write custom casing functions that pass a simpler pattern to words?

I’m not sure as to what the extent of unicode support in Liquid variable names is / should be.

@thagxt @tmmgrafikr I’ve opened an issue about numbers, in my opinion first_3d_model should be accepted as correct, as it follows a long-standing naming convention in Shopify themes:

#397

I looked into customizing it however as the current implementation relies on lodash, we would need to change the RegEx that matches against unicode words, which is beyond my RegEx skills personally:

https://github.com/lodash/lodash/blob/main/src/.internal/unicodeWords.ts

Interestingly, the words function switches to using unicodeWords once it encounters a number in front of a word or a number following a word, otherwise it uses the simpler asciiWords function. If I edit words to force asciiWords, the words array returned is ['first', '3d', 'model'] and the snakeCase result does become first_3d_model. However, the return value of unicodeWords is ['first', '3', 'd', 'model'], which results in first_3_d_model.

So there’s a mismatch in results there between the two lodash functions. words can also take a custom RegEx pattern as the second argument, I wonder if the way around this is to write custom casing functions that pass a simpler pattern to words?

I’m not sure as to what the extent of unicode support in Liquid variable names is / should be.

A custom RegEx might be the way to go here. Thinking of adding another rule, that can toggle the number formatting, to allow the other use as well.

Will look into this one when I have time, but you're all more than welcome to add a PR on this one.