Shopify / theme-tools

Everything developer experience for Shopify themes
https://shopify.dev/docs/themes
MIT License
71 stars 19 forks source link

Add `VariableName` check #359

Closed madsenmm closed 4 months ago

madsenmm commented 4 months ago

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

madsenmm commented 4 months ago

I have signed the CLA!

madsenmm commented 4 months ago

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

madsenmm commented 4 months ago

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

karreiro commented 4 months ago

Thank you, @tmmgrafikr!

thagxt commented 3 months ago

What's the problem here?

Screenshot 2024-06-27 at 12 17 31
thagxt commented 3 months ago

Documentation for this check is missing!

madsenmm commented 3 months ago

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 😅

thagxt commented 3 months ago

@tmmgrafikr Why numbers are not accepted in variable_name?

madsenmm commented 3 months ago

@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

nikitaourazbaev commented 3 months ago

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

madsenmm commented 3 months ago

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