Be stricter on eunit compilation-based warnings #650
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Looks like it's clean on rebar3 now, only rebar2 giving new trouble: Maybe |
This comment has been minimized.
This comment has been minimized.
This makes sense, since we would effectively duplicate the files for analysis. I'm testing locally and pushing a commit at the same time (again, I rebased it onto |
|
Here's the most recent output on rebar2: Quite the wall of issues, all on the test beams AFAICT. I am thinking rebar3 must just not dialyze those modules, as I can't imagine it working on rebar3 and not on rebar2 any other way. |
What's happening is that |
I further looked into this and it seems the newer
What this means is that, for a while, dialyzer for Note: I already found (via this analysis) |
Not at all—not like it's something we can control, and it's good that it's being patched in rebar3 soon. Thanks for doing this! |
The introduction of warnings_as_errors surfaced two issue classes that'll be solved in this branch's context We include this in .travis.yml so that from now on the applied strictness is maintained
Also, there's no need for ifdef TEST, in this case, since it's done by eunit by default
`?_assertNotException` (also `?assertNotException`) is to be used in cases where the Class and Term of the exception are previously known to expect not matching against. If used generically (with `_`, as-was) the Erlang pre-processor will expand that macro in such a way that a `case ... of _ -> ...; _ -> ... end` will occur thus causing an unexpected compilation issue This change fixes the compilation issue while maintaining expected behaviour
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
It seems to be behaving better now. Let's wait on Travis CI. |
Any thoughts on this? |
This comment has been minimized.
This comment has been minimized.
I endorse reducing pointless repetition. |
According to the doc.s (https://docs.aws.amazon.com/AmazonECS/latest/APIReference/API_Service.html) createdAt is "The Unix timestamp for when the service was created." which is a pos_integer(), not a float() I also update updated_at for the same reason (it's not interface breaking, it's a bugfix)
1. we remove check-unit (added by us) and concentrate the analysis in check (already present) 2. we rename eunit_warnigs (added by us) as warnings and concentrate the analysis there 3. we change rebar.config.script so we can accept an environment variable to control erl_opts (as a kind of approach to a rebar3 profile)
This comment has been minimized.
This comment has been minimized.
|
Thanks so much @paulo-ferraz-oliveira! This is a really cool improvement, and I'm really impressed by how much inconsistency/copy-pasta had gotten through without this strictness. Definitely evidence of its value. I'll give this a final (I hope) look later today, and I'll also ask @motobob and @kkuzmin if they want to take a glance at this. It is a bit large of a change for me to just merge alone, but I agree we should get this in ASAP due to its scope. |
|
Sorry about the late commit. I just remembered we wanted to start testing on Erlang/OTP 23, also. |
|
Chatted with @motobob separately, he is OK with my approval, so I am merging this. I don't think I'll release a hex package for this unless you see a need for that. Thanks again! |
I do have a need for it :) [not specifically this PR but the 3 previous ones] I'm updating an internal lib. to run on OTP 23 and would be grateful if a tag were available. |
We stop compilation with error the same we would for normal code (
warnings_as_errors).We apply this to the Travis checks.
We fix the issues surfaced by this increased strictness.