-
Notifications
You must be signed in to change notification settings - Fork 147
Rename recommended
config
#182
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
Comments
I am still having issues to understand the meaning by saying that a rule is My thought was that if a rule was
Like, any user can configure the rules they want, but the plugin offers a good "default" with what people behind it consider should be good practices everyone should follow (hence they are So in that sense
It's obvious as it is recommended by this library in particular and
I still don't see how it affects that. But now with your comment here and the fact that this issue was created, it seems there's another meaning implicit in What's the meaning it's expected to be understood when a rule is |
It's supposed to be used for the default recommended ruleset, just like |
Just to clarify if I understood correctly when you mean
you are referring to the different implementations right (react, angular, vue)? I haven't used any of them, except for the react implementation, so sometimes I forgot they are two different things 😄 So in that sense, the rules are for the general Thanks in advance 😄 |
I think the problem is this plugin is targeting different Testing Library frameworks. Usually ESLint plugins target a specific library so is pretty clear what the recommended config means. Here I'm not so sure now about what recommended means or should mean:
The thing is the plugin is mixing these two approaches so it's confusing. I prefer then to rename |
By the way, what does the |
I see. Thanks for the detailed explanation to both 😃 ; indeed there are some mixing concerns and We could add to the docs that If a rule has the I understand that they won't be required, as those should be used for rules that are specific to those implementations. |
To be honest: I thought that would happen automagically when I created the plugin but it's obviously not 😂 . I'd like to use that bool (and some extra for each config) for autogenerating that in the future. |
I want to avoid this actually. Testing Library packages like cypress or react-native are not built on top of DOM exactly. So I'd leave as many badges as needed for rules recommended on That doesn't mean we need to do it manually in the plugin code. We can leave it as now and inherit |
I don't think that system would work for us with multiple configs, I'd still like to figure out some way to declare the configs a rule belongs to in the rule so it can be used to generate all the configs and the table (not just for dom/recommended). That being said we're not the only ESLint package with multiple configs, so I wonder if there's already an example of this out there. |
+1 for the rename to It should be possible generate the configs based on the rules. |
I also did something similar here, totally forgot 😅 testing-library/eslint-plugin-jest-dom#4 Do we want to make these automation changes before or with the config renaming? |
I guess we can address it after this PR? It's a chore change so it won't mean anything for final user, while this config renaming is definitely a change for users. If you are happy with it, I'm gonna rename the config and then I can address the automation thing in a later step. |
I might be interested in giving it a shot, but I guess you're right that the order doesn't matter much. We're not changing too many things in the v4 configs yet anyway. |
🎉 This issue has been resolved in version 4.0.0-beta.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
🎉 This issue has been resolved in version 4.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Motivated by this comment I want to propose renaming
recommended
config todom
.I picked that name because is the usual config included in an ESLint plugin, but it doesn't look like proper name and seems misleading. I have seen some people too configuring
recommended
+react
or another framework, so the name can be definitely improved.My proposal then is to rename
recommended
todom
so 1) it's obvious it's not a recommended one in general, but specific one for DOM Testing Library and 2) it makes clearer the fact you don't need to enable that one together with another one for a Testing Library framework.I can address this one myself tomorrow (including doc updates) and add it to v4, but I'd like to know your opinion first.
The text was updated successfully, but these errors were encountered: