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

fix: position of reporting ESLint rule no-unnecessary-generics #794

Merged
merged 5 commits into from
Nov 2, 2023

Conversation

johnnyreilly
Copy link
Contributor

This PR addresses #734 and is a result of pairing with the ever kind and generous @JoshuaKGoldberg.

When working on the formatting of Definitely Typed, we happened upon some surprising behaviour around the no-unnecessary-generics rule. In the words of George Bernard Shaw: "I often quote myself. It adds spice to my conversation." Let me quote me now:

You'll note that we have an eslint-disable-next-line just before the last line; the ReturnType of the method. Without this in place the red squigglies would be under:

JQuery<TElement>
       ~~~~~~~~

This isn't the useful place to have this report. Perhaps the better place would be the first mention of TElement, like so:

    <TElement extends HTMLElement = HTMLElement>(
     ~~~~~~~~

This PR implements the suggested change; moving the position of the report to the type parameter position.

What do you think?

@@ -54,7 +54,7 @@ const rule = createRule({
context.report({
data: { name },
messageId: "sole",
node: parserServices.tsNodeToESTreeNodeMap.get(res.soleUse),
Copy link
Member

Choose a reason for hiding this comment

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

I think you should be able to drop soleUse or simplify otherwise, actually; I think it's only returned for this particular case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lemme have a looksee - just grabbing some dinner

Copy link
Contributor Author

Choose a reason for hiding this comment

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

simplified - think this is what you were suggesting?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, even further than I was suggesting, even! (Which is not bad.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's go even further 🔪 no need for an interface with just the one property. type Result = "ok" | ...!

Copy link
Contributor

Choose a reason for hiding this comment

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

...or even MessageId | undefined 🔪 🔪

Copy link
Contributor Author

Choose a reason for hiding this comment

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

gosh you guys are brutal. Let me sharpen the knife

Copy link
Contributor Author

Choose a reason for hiding this comment

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

how's this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks great!

feat: 🔪

nice touch :)

@jakebailey
Copy link
Member

I am personally all for this, though I am guessing it errored on the use just so that you could easily see where the single use was. But, I don't personally think that's too helpful as usually the fix is to remove the generic, thereby making the one use error anyway.

@JoshuaKGoldberg
Copy link
Contributor

Yeah I bet a good followup would be to add a suggestion for removing it.

@jakebailey
Copy link
Member

FWIW you should also write a changeset for this PR (pnpm changeset, and just let it do a patch bump). We don't have the bot set up (because it would tell everyone modifying the dependency allowlist to add changelist which we don't want); sorry about that.

@johnnyreilly johnnyreilly force-pushed the fix/no-unnecessary-generics branch from c9e1795 to 32479d8 Compare October 31, 2023 04:21
@johnnyreilly
Copy link
Contributor Author

Added the changeset - hope that's right?

@jakebailey
Copy link
Member

Seems correct to me.

Copy link
Member

@jakebailey jakebailey left a comment

Choose a reason for hiding this comment

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

This seems fine to me, but curious what others think.

Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

(in case this is helpful at all)

@sandersn sandersn merged commit ad779df into microsoft:master Nov 2, 2023
@johnnyreilly johnnyreilly deleted the fix/no-unnecessary-generics branch November 6, 2023 18:09
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.

4 participants