Skip to content

Refactor/code quality#2963

Merged
sylvestre merged 10 commits intouutils:mainfrom
danieleades:refactor/code-quality
Jan 30, 2022
Merged

Refactor/code quality#2963
sylvestre merged 10 commits intouutils:mainfrom
danieleades:refactor/code-quality

Conversation

@danieleades
Copy link
Contributor

@danieleades danieleades commented Jan 30, 2022

Addresses a number of code quality lints.

I haven't added any additional lints/checks to enforce these changes. Is that something that you want?

@danieleades danieleades force-pushed the refactor/code-quality branch from af35338 to 16bf055 Compare January 30, 2022 12:10
@sylvestre
Copy link
Contributor

I like most of the change. thanks :)
However, I am not convinced by the match => if
In general, I prefer the match

Could you please remove this commit from the pr?
thanks

@danieleades danieleades force-pushed the refactor/code-quality branch from 16bf055 to 715fcc3 Compare January 30, 2022 13:01
@danieleades
Copy link
Contributor Author

I like most of the change. thanks :) However, I am not convinced by the match => if In general, I prefer the match

Could you please remove this commit from the pr? thanks

sure thing. I've removed that commit.

@danieleades danieleades force-pushed the refactor/code-quality branch from 70c1609 to ba45fe3 Compare January 30, 2022 14:08
@danieleades danieleades marked this pull request as ready for review January 30, 2022 14:09
@tertsdiepraam
Copy link
Member

tertsdiepraam commented Jan 30, 2022

Hey! Great job! This is getting pretty big, let's split it here and put further changes in another PR. Oh you we're faster than me :)

@danieleades
Copy link
Contributor Author

Hey! Great job! This is getting pretty big, let's split it here and put further changes in another PR. Oh you we're faster than me :)

sure. happy to slice and dice this however you like! Will move further changes to a stacked PR

Copy link
Member

@tertsdiepraam tertsdiepraam left a comment

Choose a reason for hiding this comment

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

Looks all good to me!

I haven't added any additional lints/checks to enforce these changes. Is that something that you want?

Possibly, though probably not for all of these changes. For example, always using Default might hurt readability in some cases, because it's less explicit and requires contributors to know the default value for a lot of (possibly obscure) types. I'm fine with sticking to clippy's default settings for now.

@danieleades
Copy link
Contributor Author

CI failures...
I don't think they're related to this PR. how stable is the CI normally?

@tertsdiepraam
Copy link
Member

It's pretty stable these days, but it fails every once in a while. It seems unrelated indeed. I'll rerun the CI

@danieleades
Copy link
Contributor Author

It's pretty stable these days, but it fails every once in a while. It seems unrelated indeed. I'll rerun the CI

loads of failures in GnuTests job. Still think it's unrelated to this PR? I can split the commits into separate PRs if it helps?

@tertsdiepraam
Copy link
Member

loads of failures in GnuTests job

That's to be expected. This tests against all GNU tests and we are not 100% compatible. The job will fail if there are any tests that pass on main but not on the PR.

@tertsdiepraam
Copy link
Member

I've created an issue for the test that failed: #2969

@danieleades
Copy link
Contributor Author

@tertsdiepraam
finished! that's some epic CI

@sylvestre
Copy link
Contributor

what tool did you use for that btw?

@danieleades
Copy link
Contributor Author

what tool did you use for that btw?

it was about 5% manual effort, and about 95% clippy

@sylvestre
Copy link
Contributor

I guess it is with inactivated options of clippy as we have it in the CI?
Could you please update the CI to enable them?
thanks

@sylvestre sylvestre merged commit de07df5 into uutils:main Jan 30, 2022
@danieleades danieleades deleted the refactor/code-quality branch January 30, 2022 17:31
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.

3 participants