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

add no-dupe-class-members rule #82

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

alidn
Copy link

@alidn alidn commented Jan 14, 2021

Checklist

  • It does not work if a method name is in the form of 'name' or ['name']
  • Clean-up and maybe offer better diagnostic messages

@RDambrosio016
Copy link
Collaborator

Doesn't this have to account for typescript overloading?

@RDambrosio016 RDambrosio016 added C-enhancement Category: enhancement T-Runner This issue primary relates to the lint runner, rslint_core labels Jan 14, 2021
@alidn
Copy link
Author

alidn commented Jan 14, 2021

That's right. Sorry I wasn't even considering TypeScript support, I'll fix that

@RDambrosio016
Copy link
Collaborator

Don't forget to run cargo docgen to make the md file with the docs for the website once you're done too 👍

}
}

fn check<T: Named>(code: &str, ctx: &mut RuleCtx, nodes: Vec<T>) -> Option<Diagnostic> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
fn check<T: Named>(code: &str, ctx: &mut RuleCtx, nodes: Vec<T>) -> Option<Diagnostic> {
fn check(code: &str, ctx: &mut RuleCtx, nodes: Vec<impl Named>) -> Option<Diagnostic> {

}
if let Some(prev_decl) = names.insert(name.clone(), n.name_node()) {
let err = ctx
.err(code, format!("Duplicate name '{}'", name))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.err(code, format!("Duplicate name '{}'", name))
.err(code, format!("Duplicate name `{}`", name))

We use backticks for code/names in the project usually, so this should be consistent too

.err(code, format!("Duplicate name '{}'", name))
.secondary(
prev_decl,
format!("Previous declaration of the method '{}' here", &name),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
format!("Previous declaration of the method '{}' here", &name),
format!("Previous declaration of the method `{}` is here", &name),

See above

prev_decl,
format!("Previous declaration of the method '{}' here", &name),
)
.primary(n.name_node(), format!("'{}' redefined here", name));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.primary(n.name_node(), format!("'{}' redefined here", name));
.primary(n.name_node(), format!("`{}` is redefined here", name));

See above

@alidn
Copy link
Author

alidn commented Jan 14, 2021

It does not handle class properties, and I'm not sure if it should. Looks like typescript-lint doesn't handle it either.

@RDambrosio016
Copy link
Collaborator

I think it should considering class fields are supported by default in the parser under ts syntax, and in the future i'll allow it in js files with a config flag too

@@ -32,4 +32,5 @@ group! {
valid_typeof::ValidTypeof,
no_extra_boolean_cast::NoExtraBooleanCast,
no_confusing_arrow::NoConfusingArrow,
no_dupe_class_members::NoDupeClassMembers
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that I see the list of rules here, I think it would be pretty nice to sort them in alphabetic order.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: enhancement T-Runner This issue primary relates to the lint runner, rslint_core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants