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

cargo: enable ‘-D warnings’ by default #6254

Closed
wants to merge 1 commit into from
Closed

Conversation

mina86
Copy link
Contributor

@mina86 mina86 commented Feb 7, 2022

Making ‘-D warnings’ the default may be a minor annoyence to developers
who now will need to fix warnings even while they work on the feature
(as opposed to only having to do that once they are ready to send the
code for review) but it does simplify the code a small amount.

Issue: #6226

Making ‘-D warnings’ the default may be a minor annoyence to developers
who now will need to fix warnings even while they work on the feature
(as opposed to only having to do that once they are ready to send the
code for review) but it does simplify the code a small amount.

Issue: near#6226
Copy link
Collaborator

@bowenwang1996 bowenwang1996 left a comment

Choose a reason for hiding this comment

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

Is there a reason for removing the check from CI? I suppose we should still check for warnings in CI to make sure they are fixed

@mina86
Copy link
Contributor Author

mina86 commented Feb 7, 2022

With the flag being set in .cargo/config.toml setting RUSTFLAGS is now redundant.

@matklad
Copy link
Contributor

matklad commented Feb 8, 2022

I don't think we should do this. To me, it seems that while the annoyance can be minor, it can also be very significant.

For example, I often do quick&dirty exploratory work (building the code with two versions of some library, etc) where I have a ton of warnings.

Even if it were true that the annoyance is minor, i'd argue that fixing a minor, repeated annoyance for everyone is well worth to spend a couple of extra CI lines on.

There's also a point that we are setting RUST_BACKTRACE=1 anyway, so it's not like we make anything meaningfully simpler, we still have the repeated "set up common env" step. It seems to be that the actual problem here is that we can't set env-variable once, like we can do with github actions:

https://github.com/rust-analyzer/rust-analyzer/blob/9b1978a3ed405c2a5ec34703914ec1878b599e14/.github/workflows/ci.yaml#L10-L16

@mina86 mina86 closed this Feb 8, 2022
@mina86 mina86 deleted the warn branch February 8, 2022 12:40
@mina86
Copy link
Contributor Author

mina86 commented Feb 8, 2022

It seems to be that the actual problem here is that we can't set env-variable once, like we can do with github actions:

Actually it can: #6269

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.

None yet

3 participants