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

Added copy lint name button to lint list #8473

Conversation

Mastermindaxe
Copy link

Fixes #7959

changelog: Added 'copy lint name' button to lint list

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @camsteffen (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Feb 25, 2022
@Mastermindaxe
Copy link
Author

I added a tooltip for the button and an alert for when you've clicked it

@xFrednet
Copy link
Member

xFrednet commented Feb 25, 2022

I wanted this for quite some time now ❤️. I find the alert a bit distracting, but we should give some kind of visual feedback. Maybe an icon change like rustdoc has for items would be better.

image -> image

rustdoc uses .svg images for that, we could also consider doing that if icons don't look so good. I think the current clipboard emoji is a little to small 🤔


Edit: Also, I think we should discuss if the name should be prefixed with clippy:: or not. For discussions, I usually just write the lint name without the prefix, but I expect this feature to be used by people on the fly to enable/disable a lint. Then it makes more sense to add the clippy:: prefix to use the lint directly in lint attributes 🤔

@camsteffen
Copy link
Contributor

I agree we should avoid alert and there should be something that visually indicates "click here to copy to clipboard".

I also think it would be good to prefix clippy::, even if it's just for the clipboard and not actually on the page.

@camsteffen camsteffen added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Feb 26, 2022
@Mastermindaxe
Copy link
Author

Mastermindaxe commented Feb 28, 2022

Sounds like a plan. I'm gonna try to get these changes in over the next few days

@camsteffen What exactly do you mean by prefixing? Just putting clippy:: in front of the lint name in the clipboard? Wouldn't that defeat the purpose?
EDIT: Just saw the edited message by @xFrednet. Interesting point 🤔 Not sure how to solve this or decide on one way or the other

@xFrednet
Copy link
Member

xFrednet commented Feb 28, 2022

Thank you! To solve this, you can just add the prefix in line 504 like this (Or any other variation of JS string formatting.):

- navigator.clipboard.writeText(lint.id);
+ navigator.clipboard.writeText("clippy::" + lint.id);

I believe that most people would expect the prefix to be there. In discussions in this repo we often leave it out as the domain is clear, but having it wouldn't hurt. So, with some consideration, I would suggest prefixing it every time. 🙃

A second option would be to have a setting or key combination to control the prefix, but I believe that that is more confusing than helpful for 99% of the users.

If there are complaints / suggestions to remove the prefix it's an easy change :)

@xFrednet
Copy link
Member

Hey @Mastermindaxe, are you still working on this? If you need any support, we're happy to help. 🙃

@xFrednet xFrednet assigned xFrednet and unassigned camsteffen Mar 25, 2022
@Mastermindaxe
Copy link
Author

Sorry for being inactive so long. Have been swamped with work and private stuff. I'm hoping that I can squeeze it in some time in the next two weeks. No promises but I'm trying!

@xFrednet
Copy link
Member

No problem @Mastermindaxe and thank you for the response, if you're busy, you can also reach out. I'd be happy to add the finishing touches and ask another team member to review the final work 🙃

@xFrednet
Copy link
Member

I'm closing this due to the long inactivity. If you have the time, feel free to reopen it 🙃.

Thank you very much for starting this implementation. I want to suggest this issue to another contributor and will reference this PR along with your work.

@xFrednet xFrednet closed this Apr 28, 2022
@xFrednet xFrednet added S-inactive-closed Status: Closed due to inactivity and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Apr 28, 2022
@blyxyas blyxyas removed the S-inactive-closed Status: Closed due to inactivity label Oct 9, 2023
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.

Add copy lint name function to Clippy's lint list
6 participants