43081j / eslint-plugin-lit

lit-html support for ESLint
120 stars 22 forks source link

feat: add strictSnakeCase option to rule attribute-names #207

Closed jpradelle closed 2 weeks ago

jpradelle commented 1 month ago

I would like to force attribute name to be the exact snake-case version of property, like PolymerJS did, I added option strictSnakeCase to attribute-names rule to check that.

With that option, the only possible attributes values are

@property({attribute: 'camel-case-name'})
camelCaseName: string;

// or
@property({attribute: false})
camelCaseName: string;
43081j commented 1 month ago

i feel like if we're to do this, we should provide a more generic option to account for the current behaviour too

e.g. style: "snake" | "none" (none being the default, in that the attr name is the same as the prop name but lowercased)

jpradelle commented 1 month ago

Here it is with style option. By default no style is applied, same behavior a before. I also fixed naming issue between snake_case and dash-case

43081j commented 1 month ago

i understand better now

currently (in this PR), we have two behaviours:

the confusion here is that default differs from everything else, including none

maybe we should rename style to convention and have this behaviour:

jpradelle commented 1 month ago

I updated with convention instead of style

But I don't fully get this part :

maybe we should rename style to convention and have this behaviour:

  • no convention does the current behaviour in main (check the attr is lower case)
  • all others compare the attribute and the prop to enforce a convention (which implicitly will also enforce that it is lower case)
  • none still exists but is the default (i.e. no convention, so just remove the default in the switch because it should be impossible to reach)

It's ok for not changing current rule behavior if convention is not defined.

But I don't see how we can have a default value for convention. Either it's not specified and we keep current rule behavior or we define a convention, and to define it, it must have a value, we can not have default value for it. Check for allowed values is done here : https://github.com/43081j/eslint-plugin-lit/pull/207/files#diff-0eff54e47634b63f2db864b99a12f005fd784612af3d7b64893fbf03ae1f8bd3R25

In that case, none is maybe not very meaningful, what about renaming it something like lowercase or lower

43081j commented 1 month ago

i pushed a few changes, maybe that'll help you understand

having none and a default (null) doesn't make much sense. the default is none, in that we don't enforce a convention

what i was trying to point out was that these two things are separate:

all attributes, regardless of convention, need to be lowercase. in the commit i pushed, i've moved the code out of the condition so it always runs.

separately, if the attribute was lowercase, we then enforce a particular naming convention if one is set

jpradelle commented 2 weeks ago

I pushed a small documentation update, are you requesting more updates on this PR ?

43081j commented 2 weeks ago

Looks good to me!

Thanks a lot for seeing this through 🙏