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

Fixing unneeded return linting errors #24

Merged
merged 1 commit into from Nov 10, 2018

Conversation

Projects
None yet
3 participants
@Mordax
Contributor

Mordax commented Nov 9, 2018

Closes #5 . Should remove the annoying failed build badge.

@SeanPrashad

This comment has been minimized.

Contributor

SeanPrashad commented Nov 9, 2018

Great stuff @Mordax! Travis should pass with a green checkmark but it looks like there's some other things breaking the Beta builds.

Check them out here! The error points to Clippy docs: https://rust-lang-nursery.github.io/rust-clippy/v0.0.212/index.html#new_ret_no_self

Looping in @0xazure - do we want to have failing Beta builds fail for Travis?

@0xazure

This comment has been minimized.

Owner

0xazure commented Nov 9, 2018

Okay, this is interesting. The clippy lint in question is new_ret_no_self, which should have failed on stable as well.

I unfortunately can't dive any deeper on this right now because of rust-lang/rust#55817 so I can't actually install the beta channel to test this locally.

@0xazure

This comment has been minimized.

Owner

0xazure commented Nov 9, 2018

I'm not actually too fussed about the beta failure because I remove the offending method in #19, but I'd still like to figure out why the two channels are seemingly running different lints against the codebase.

Update: older jobs on beta didn't trigger this lint, so I'm rerunning the beta builds to see what happens. Builds still fail, investigating...

@SeanPrashad

This comment has been minimized.

Contributor

SeanPrashad commented Nov 9, 2018

Luckily we have friends over at #rust:

irc_1

irc_2

irc_3

@0xazure

This comment has been minimized.

Owner

0xazure commented Nov 9, 2018

clippy on beta is a newer clippy than the clippy on stable

I wish it were that simple. It looks like the new_ret_no_self lint is relatively new (within the last 30-35 days) and experienced a bunch of false positives with the initial implementation. As far as I can tell, stable doesn't include this lint at all, beta has the version with false positives, and nightly has the updated version that eliminates a lot of the false positives seen on beta, which explains why we're only seeing failures on beta.

@0xazure

0xazure approved these changes Nov 9, 2018

Given the above ☝️ and the fact that the tests pass on stable, LGTM 👍 :shipit:

Congrats @Mordax on your first Rust code contribution to the project!!! 🎉 Can't wait to see what's next from you.

@0xazure 0xazure merged commit 2288e5c into 0xazure:master Nov 10, 2018

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
@Mordax

This comment has been minimized.

Contributor

Mordax commented Nov 10, 2018

Clearly what's next is a billion dollar app 😎

0xazure added a commit that referenced this pull request Nov 14, 2018

bug: Do not run clippy on beta toolchain
As observed in #24, only invoke Clippy for the `Stable` and `Nightly` release channels on Travis CI.

Unfortunately, versions of Clippy vary across channels. Only run Clippy against the `Stable` channel, as well as `Nightly` which is allowed to fail. The `Beta` release channel is still tested against, but only for crate tests and not code linting.

Closes #25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment