Skip to content
This repository has been archived by the owner on Aug 1, 2022. It is now read-only.

fix: clippy CI #430

Merged
merged 5 commits into from
May 28, 2020
Merged

fix: clippy CI #430

merged 5 commits into from
May 28, 2020

Conversation

rudolfs
Copy link
Member

@rudolfs rudolfs commented May 27, 2020

As per workaround documented in: rust-lang/rust-clippy#4612

This adds 11m to the build however : (
We should probably try to upgrade to the latest nightly version and try -Zunstable-options.

Closes #376.

@rudolfs rudolfs force-pushed the rudolfs/fix-clippy branch 2 times, most recently from e046235 to 1502bbf Compare May 27, 2020 13:50
@rudolfs rudolfs requested a review from xla May 27, 2020 13:53
@rudolfs rudolfs marked this pull request as ready for review May 27, 2020 13:53
@xla xla changed the title Fix clippy on CI fix: clippy CI May 27, 2020
@xla
Copy link
Contributor

xla commented May 27, 2020

A better way that doesn't increase the build time is to get rid of the check step, which is entailed in the clippy command. This would avoid the clean.

We should probably try to upgrade to the latest nightly version and try -Zunstable-options.

What you hope to gain from this? Or what will this impact?

@rudolfs
Copy link
Member Author

rudolfs commented May 27, 2020

A better way that doesn't increase the build time is to get rid of the check step, which is entailed in the clippy command. This would avoid the clean.

What does the check do and why do we need it in the first place?

What you hope to gain from this? Or what will this impact?

Apparently they fixed the bug and with that flag clippy should work and still use the cache. Read more about it here:
rust-lang/rust-clippy#3837 (comment)

@xla
Copy link
Contributor

xla commented May 27, 2020

What does the check do and why do we need it in the first place?

It's the cargo builtin linter. clippy is a superset tho, so running either should be enough, In our case clippy preferrably.

Apparently they fixed the bug and with that flag clippy should work and still use the cache. Read more about it here:

Great, we run a reasonably recent nightly so the flag should be supported.

@rudolfs
Copy link
Member Author

rudolfs commented May 27, 2020

It's the cargo builtin linter. clippy is a superset tho, so running either should be enough, In our case clippy preferrably.

In that case I'll try to remove cargo check first and see if that gets clippy to work on CI again.

@rudolfs
Copy link
Member Author

rudolfs commented May 27, 2020

That seems to work and the build only takes 30s to fail on clippy.
Next step is fixing all the clippy complaints in this PR.

@xla
Copy link
Contributor

xla commented May 27, 2020

That seems to work and the build only takes 30s to fail on clippy.

Glad my initial suggestion worked out. Let me know if you need help addressing the actual clippy complains.

Copy link
Contributor

@xla xla left a comment

Choose a reason for hiding this comment

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

Would revert the unwrap change, otherwise gud.

🍭 🔁 🌒 🕒

proxy/src/http/transaction.rs Outdated Show resolved Hide resolved
@rudolfs rudolfs merged commit 9729b73 into master May 28, 2020
@rudolfs rudolfs deleted the rudolfs/fix-clippy branch May 28, 2020 11:44
rudolfs added a commit that referenced this pull request Aug 3, 2020
This was first fixed in:
#430.

But then later broken in:
#721
rudolfs added a commit that referenced this pull request Aug 3, 2020
This was first fixed in:
#430.

But then later broken in:
#721
rudolfs added a commit that referenced this pull request Aug 3, 2020
* Make clippy work on CI again

This was first fixed in:
#430.

But then later broken in:
#721

* Appease clippy
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Actually run clippy in builds
2 participants