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

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

Merged
merged 6 commits into from
Aug 29, 2024

Conversation

jpradelle
Copy link
Contributor

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

43081j commented Jul 29, 2024

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

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

43081j commented Jul 30, 2024

i understand better now

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

  • default - just check that the attribute is lowercase or there isn't one, and prop is lower case
  • none - check that the attribute is the lowercase of the prop name
  • snake - check that the attribute is the snake_case of the prop name
  • kebab - check that the attribute is the kebab-case of the prop name

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

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)

@jpradelle
Copy link
Contributor Author

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

43081j commented Aug 2, 2024

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:

  • enforcing that attributes are lowercase
  • enforcing a naming convention of attributes

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

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

@43081j
Copy link
Owner

43081j commented Aug 29, 2024

Looks good to me!

Thanks a lot for seeing this through 🙏

@43081j 43081j merged commit 8e18aae into 43081j:master Aug 29, 2024
3 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants