Remove redundant _linter suffix from auto names?

[AshesITR]

The new # nolint: xxx. feature makes it more likely for linter names to appear in source code.
I would therefor suggest we remove the redundant _linter suffix from the names to make most lint exclusions look more natural.
Compare:

my.bad.name <- function(a, b) { # nolint: object_name_linter.
  # Very long comment explaining why the name came to be and needs to stay # nolint: line_length_linter.
  a + b
}

to

my.bad.name <- function(a, b) { # nolint: object_name.
  # Very long comment explaining why the name came to be and needs to stay # nolint: line_length.
  a + b
}

wdyt?

cc @MichaelChirico @jimhester

[MichaelChirico]

to clarify, you just want the #nolint logic to automatically add _linter to the set of names it finds?

the first thing that comes to mind is that custom linters may not be snake case, e.g. we use UpperCamelCase for all functions, including our linters.

[AshesITR]

I don't want to change the behavior of nolint. Instead I propose to change the automatically generated linter names (in with_defaults and in Linter) such that they don't include the _linter suffix.

User-visible changes here are the linter names in lints lists and the interface for nolint sections, as well as changes necessary in the .lintr config if a default linter was overridden.

Feel free to check out the draft PR to see what exactly changes.

[jimhester]

I don't think changing the function names is a good idea, but the nolint parser could probably add the _linter suffix if you think that looks better in the comments.

[AshesITR]

We're not changing the function names, we're changing the names in the linters list, used for exclusions.
All functions keep their original name (with the _linter suffix)

[MichaelChirico]

please add tests of with_defaults & exclusions where a custom linter doesn't have the _linter suffix, if there aren't any yet

[MichaelChirico]

@AshesITR added this to 3.0.0 milestone since it sounds like we should make some decision before release. I recall you said this might be obsolete.

[AshesITR]

I closed the initial draft.
Do you think it's worthwhile for exclusions to automagically add the _linter suffix when parsing exclusions?
If not, feel free to close this issue and we stick with the original implementation.

[MichaelChirico]

Do you think it's worthwhile for exclusions to automagically add the _linter suffix when parsing exclusions?

Would that work like:

# nolint: foo.

excludes both foo and foo_linter if found among active linters?

I could be OK with that. Is there any equivalent in, say, pylint?

[AshesITR]

Just had another thought about this.
What if we allow the # nolint: ... lists to exclude by any unique prefix (i.e. using pmatch() on the active linters)?

This would also benefit other naming schemes which use suffixes (e.g. a hypothetical CamelCaseLinter) and be easy to document.

@MichaelChirico what are your thoughts on this?
I could whip up a PR for closer inspection.

[MichaelChirico]

it does sound better; the downside is if it'll be abused?

nolint: expect_nu

would match expect_null_linter

OTOH if downstream want to be overly concise maybe that's up to them? or we could use an option to disable that (like warnPartialMatchDollar)

[AshesITR]

Adding a switch seems like a viable solution if downstream complains about this feature.
I expect users of lintr to generally be interested in code readability, so I'm not too worried about abuse of the notation.

We should definitely warn if an exclusion couldn't be matched (e.g. due to non-uniqueness), though.

[MichaelChirico]

SGTM