Skip to content

Include custom clippy settings #867

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

Merged
merged 7 commits into from
May 25, 2023
Merged

Include custom clippy settings #867

merged 7 commits into from
May 25, 2023

Conversation

nyurik
Copy link
Contributor

@nyurik nyurik commented May 25, 2023

Help needed: no idea why tests fail in all of my PRs :(

  • All custom clippy settings are now centralized
  • add make fix-clippy for automated fixes
  • Modified some simple cases, e.g. use _ instead of true|false in a match

Unrelated to this specific change, but I would highly recommend to use justfile -- much cleaner and easier.

Also, I think it would be better to use cargo -p <project> in the makefile and elsewhere - this way it will be cleaner to repeate the broken CI step without the cd ..., and also will not require the test to contain some non-buildable paths - see #865

* All custom clippy settings are now centralized
* add `make fix-clippy` for automated fixes
* Modified some simple cases, e.g. use `_` instead of `true|false` in a match
Copy link
Member

@Byron Byron left a comment

Choose a reason for hiding this comment

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

Thanks a lot (for all these improvements)!

I am loving the new clippy target and will merge once having sorted the makefile TODO. Also I will re-evaluate the justfile, even though last time I still preferred the simplicity and ubiquitousness of makefiles.

Also, I think it would be better to use cargo -p <project> in the makefile and elsewhere - this way it will be cleaner to repeate the broken CI step without the cd ..., and also will not require the test to contain some non-buildable paths - see #865

I will also take a look at this, and agree that -p is underused here. Admittedly, back in the days I wasn't aware of it.

@Byron
Copy link
Member

Byron commented May 25, 2023

Regarding just, I think it could work and I am keen to convert, despite the general shortcoming that it isn't installed automatically, but installing it should help to get there in the future :).

I check the manual to get some issues clarified:

It's a bit sad to see that the listing isn't as pretty as the one-liner that is used here with make :/, but I find comfort that I can contribute headlines myself once I am fed up enough 😁.

Screenshot 2023-05-25 at 10 43 12 Screenshot 2023-05-25 at 10 45 50

Yes, so I think I will have some fun porting the makefile to a justfile - thanks for reminding me!

@Byron
Copy link
Member

Byron commented May 25, 2023

Thanks again for all your work here!

I will see if not changing directory justfile makes it more readable, but when trying here I thought it was preferable still. I have ideas though and think this can get better no matter what.

I will start a new PR for transcribing the makefile in a moment right after merging once CI is green, excitement!

@Byron Byron mentioned this pull request May 25, 2023
2 tasks
@Byron Byron merged commit 4795fcf into GitoxideLabs:main May 25, 2023
@nyurik nyurik deleted the auto-clippy branch May 25, 2023 16:41
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.

2 participants