Skip to content

DO NOT MERGE: warnings #109

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

Closed
wants to merge 4 commits into from
Closed

DO NOT MERGE: warnings #109

wants to merge 4 commits into from

Conversation

petertseng
Copy link
Member

@petertseng petertseng commented Apr 16, 2016

Staging ground to test some refinements to #107 as well as intentionally adding some warnings to test whether the changes are working as expected. Do not merge.

if [ $status -ne 0 ]; then
# Instead of immediate exit, we keep going.
# This allows us to see all warnings.
exitcode=1
Copy link
Member Author

Choose a reason for hiding this comment

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

hmm, isn't actually causing exit. wonder why.

Copy link
Member Author

Choose a reason for hiding this comment

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

subshells can't affect their parent shell's vars, of course. have to return it.

I don't intend to be terribly strict on warnings (so as to actually fail
a build with them) but I think I'd like a way to see them, so I'll let
Travis do the job.

This is motivated by the fact that I introduced warnings in #81 and they
were not caught.
This requires not setting -e for the `cargo test` run, nor exiting early
in the manual status check.

The motivation is that we'd like to see all warnings in this mode, not
just the first exercise with warnings.
In DENYWARNINGS I just want to see all the warnings; I don't care about
whether the tests actually pass or what I'm compiling or downloading.
This tests whether multiple warnings are showing up.
@@ -1,3 +1,5 @@
use std::convert::AsRef;
Copy link
Member Author

Choose a reason for hiding this comment

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

as luck would have it, this is not a warning. word_count has a legitimate use of as_ref. Doesn't matter, I've seen what I need to see.

@petertseng petertseng closed this Apr 16, 2016
@petertseng petertseng deleted the DO-NOT-MERGE-warnings branch April 16, 2016 13:14
@petertseng petertseng restored the DO-NOT-MERGE-warnings branch April 16, 2016 13:14
@petertseng petertseng deleted the DO-NOT-MERGE-warnings branch July 2, 2016 22:19
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.

1 participant