Deny warnings #47

llogiq · 2016-12-21T20:01:21Z

This fixes #46

llogiq · 2016-12-21T20:01:53Z

Oops, somehow messed up the history.

nrc · 2016-12-21T20:09:56Z

anti_patterns/deny-warnings.md

+## Drawbacks
+
+Until, say, the language changes, and as part of the transition, a lint is
+introduced that `warn`s, and Boom! The code no longer builds. Or someone tries


I would phrase this in terms of the Rust language's back compat guarantees - we guarantee (mostly) that we won't break your code with new releases, however, that guarantee does not cover warnings, and thus by using deny(warnings) you are opting out of backwards compatibility for your crate.

I've changed this to reflect your suggestion.

nrc · 2016-12-21T20:10:24Z

anti_patterns/deny-warnings.md

+
+## Alternatives
+
+Our plucky developer hero should have used


I would avoid this language, it sounds kinda patronising to me

You're right. I was a bit aggressive because I've had to deal with so many crates doing this, just creating an obstacle for me to test clippy on their code...it's all good now 😄

nrc · 2016-12-21T20:11:36Z

anti_patterns/deny-warnings.md

+
+- Type `rustc -W help` for a list of lints on your system. Also type
+`rustc --help` for a general list of options
+- [clippy] is a collection of lints for better Rust code


I wonder if it is worth being explicit and calling it 'rust-clippy' rather than just clippy, since the latter is technically something else (and probably TM etc. MS).

nrc · 2016-12-21T20:14:53Z

anti_patterns/deny-warnings.md

+Our plucky developer hero should have used
+`RUSTFLAGS="-D warnings" cargo build"` to build their crate, removing
+the need for the annotation in their code.
+


I would add some discussion around the fact that usually in Rust, we recommend using in-crate attributes rather than command line flags (several reasons for this, starting with ensuring consistent build experiences without needing a build script on top of Cargo or whatever). However, the advice here is an exception to that rule and maybe explain why it makes sense (which I guess comes down to, deny(warnings) whould be a personal part of the build experience, not something you force onto your clients).

It is probably worth stating the alternative of denying specific warnings as attributes which does not have the behaviour you describe.

It is probably worth stating the alternative of denying specific warnings as attributes which does not have the behaviour you describe.

Is this true in general? Even when only specific warnings are chosen, the meanings of the warnings themselves can change over time. That was part of the motivation for rust-lang/rfcs#1193, after all.

I would guess it would be 99% true. Warnings very rarely change their meaning. Though they might expand slightly, it is usually to cover edge cases and is unlikely to cause breakage. I expect that any problems this causes is more than outweighed by the benefits of catching errors earlier in the development process and without needing a custom script to run the builds. ISTM that the hazard of the lint case is another lint silently appearing and this approach does not have that problem.

All right. I think the list of lints is now complete and usable.

llogiq · 2016-12-22T21:04:56Z

I now rewrote most of the text, added a few allow lints for good measure and fixed history.

@nrc Thank you for the useful comments. I'm now much happier with this PR.

llogiq · 2016-12-22T21:40:36Z

Note to self: Perhaps I should link to the #[deprecate] docs, too.

llogiq · 2016-12-26T09:02:50Z

Ok, now I think this is ready to merge. Thoughts?

lambda-fairy

Merry Christmas!

Now that Christmas is over, I finally have the chance to properly review your PR. Hope these comments are helpful.

lambda-fairy · 2016-12-27T08:59:13Z

anti_patterns/deny-warnings.md

+       unused-extern-crates,
+       unused-import-braces,
+       unused-qualifications,
+       unused-results)]


I'd add missing-debug-implementations (and maybe missing-copy-implementations, though that's perhaps more controversial).

lambda-fairy · 2016-12-27T08:59:38Z

anti_patterns/deny-warnings.md

+       unused-results)]
+```
+
+Note that we explicitly did not set the [deprecate attribute]


Nit: there should be a period at the end of this sentence

lambda-fairy · 2016-12-27T09:02:26Z

anti_patterns/deny-warnings.md

+## Description
+
+A well-intentioned crate author wants to ensure their code builds without
+warnings. So they annotate their crate root with


Reading the rendered view, it's not obvious that the next example finishes this sentence. I'd rather have this written out in full instead.

lambda-fairy · 2016-12-27T09:04:58Z

anti_patterns/deny-warnings.md

+## Drawbacks
+
+By disallowing the compiler to build with warnings, a crate author opts out of
+Rust's famed stability. Sometimes new features or old misfeatures need a change


I would like an example of such a change -- any lint from the future-incompatible group would be okay.

lambda-fairy · 2016-12-27T09:05:13Z

anti_patterns/deny-warnings.md

+period before being turned to `deny`. Sometimes APIs get deprecated, so their
+use will emit a warning where before there was none.
+
+ALl this conspires to potentially break the build whenever something changes –


Nit: "ALl" should be "All"

lambda-fairy · 2016-12-27T09:07:30Z

anti_patterns/deny-warnings.md

+
+ALl this conspires to potentially break the build whenever something changes –
+not only for the original developer, but also for those who wish to use their
+code.


This isn't true: Cargo passes --cap-lints to rustc, which disables lints while building dependencies. See rust-lang/rfcs#1193.

lambda-fairy · 2016-12-27T09:11:55Z

anti_patterns/deny-warnings.md

+
+## Alternatives
+
+luckily, there are two ways of tackling this problem: First, we can decouple


I think the word "luckily" should be removed. These articles tend to have a sparse writing style, which discourages extra words like this.

lambda-fairy · 2016-12-27T09:26:21Z

anti_patterns/deny-warnings.md

+```RUSTFLAGS="-D warnings" cargo build"```
+
+This can be done by any individual developer (or be set in a CI tool like
+travis, but remember that this may break the build when something changes)


Nit: "Travis" should be capitalized

llogiq · 2016-12-28T23:59:19Z

Thanks @lfairy for the thorough read-through. This is becoming quite good now.

@nrc @lfairy r?

lambda-fairy · 2016-12-31T00:57:15Z

Looks good to me! I'll wait for any comments from @nrc before merging.

llogiq · 2017-01-03T21:40:33Z

@nrc ping?

nrc · 2017-01-04T00:13:54Z

LGTM, thanks for the changes!

Add another example to the mem::replace idiom

d827cec

This fixes rust-unofficial#38

nrc requested changes Dec 21, 2016

View reviewed changes

New anti-pattern: deny-warnings

b06eacd

llogiq force-pushed the deny_warnings branch from ce1eeb9 to b06eacd Compare December 22, 2016 19:28

llogiq added 2 commits December 26, 2016 09:32

added deprecated link

8c0af98

fix link title, add explanation why not deprecated

81d5d4d

lambda-fairy requested changes Dec 27, 2016

View reviewed changes

incorporated hints from lfairy

f08e132

lambda-fairy approved these changes Dec 31, 2016

View reviewed changes

nrc merged commit 1cca042 into rust-unofficial:master Jan 4, 2017

llogiq deleted the deny_warnings branch January 4, 2017 01:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deny warnings #47

Deny warnings #47

llogiq commented Dec 21, 2016

llogiq commented Dec 21, 2016

nrc Dec 21, 2016

llogiq Jan 3, 2017

nrc Dec 21, 2016

llogiq Jan 3, 2017

nrc Dec 21, 2016

llogiq Jan 3, 2017

nrc Dec 21, 2016

lambda-fairy Dec 22, 2016

nrc Dec 22, 2016

llogiq Jan 3, 2017

llogiq commented Dec 22, 2016

llogiq commented Dec 22, 2016

llogiq commented Dec 26, 2016

lambda-fairy left a comment •

edited

Loading

lambda-fairy Dec 27, 2016

llogiq Dec 28, 2016

lambda-fairy Dec 27, 2016

lambda-fairy Dec 27, 2016

lambda-fairy Dec 27, 2016

llogiq Dec 28, 2016

lambda-fairy Dec 27, 2016

lambda-fairy Dec 27, 2016

lambda-fairy Dec 27, 2016

lambda-fairy Dec 27, 2016

llogiq commented Dec 28, 2016

lambda-fairy commented Dec 31, 2016

llogiq commented Jan 3, 2017

nrc commented Jan 4, 2017


		## Alternatives

		luckily, there are two ways of tackling this problem: First, we can decouple

Deny warnings #47

Deny warnings #47

Conversation

llogiq commented Dec 21, 2016

llogiq commented Dec 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llogiq commented Dec 22, 2016

llogiq commented Dec 22, 2016

llogiq commented Dec 26, 2016

lambda-fairy left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llogiq commented Dec 28, 2016

lambda-fairy commented Dec 31, 2016

llogiq commented Jan 3, 2017

nrc commented Jan 4, 2017

lambda-fairy left a comment •

edited

Loading