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

Document compiler options warn_missing_spec and warn_missing_spec_all #2918

Merged

Conversation

paulo-ferraz-oliveira
Copy link
Contributor

@paulo-ferraz-oliveira
Copy link
Contributor Author

paulo-ferraz-oliveira commented Dec 8, 2020

It's still missing tests.

Let me know if those tests are sufficient. Mind you I'm not testing new behavior; I'm just (minimally) making sure no regressions are introduced in the future.

@paulo-ferraz-oliveira paulo-ferraz-oliveira marked this pull request as draft December 8, 2020 17:40
@paulo-ferraz-oliveira paulo-ferraz-oliveira marked this pull request as ready for review December 8, 2020 23:02
@IngelaAndin IngelaAndin added the team:VM Assigned to OTP team VM label Dec 9, 2020
@bjorng bjorng self-assigned this Dec 9, 2020
Copy link
Contributor

@bjorng bjorng left a comment

Choose a reason for hiding this comment

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

Thanks for your pull request.

The documentation looks good.

The tests are more complicated than they need to be. We usually put tests for options in the application that implements them. In this case, the spec options are implemented in erl_lint (in STDLIB) and should be tested in erl_lint_SUITE.

@paulo-ferraz-oliveira
Copy link
Contributor Author

Thanks for your pull request.

The documentation looks good.

The tests are more complicated than they need to be. We usually put tests for options in the application that implements them. In this case, the spec options are implemented in erl_lint (in STDLIB) and should be tested in erl_lint_SUITE.

Yeah. I was mislead in the bug report, but I'll fix that.

As for the tests, you prefer I make them simpler? I actually think they're as simple as possible, but I'm willing to change them, if you see it fit.

@bjorng
Copy link
Contributor

bjorng commented Dec 9, 2020

As for the tests, you prefer I make them simpler?

I mean simpler in the sense that the scaffolding in erl_lint_SUITE will allow you to do the same tests using less code and without creating four extra Erlang modules. In erl_lint_SUITE, you can just put source code to be tested in one or more binaries and call the run/2 function.

@paulo-ferraz-oliveira
Copy link
Contributor Author

I mean simpler in the sense that the scaffolding in erl_lint_SUITE will allow you to do the same tests using less code

Sure thing. Let me simplify these proper. Thanks for your help.

@paulo-ferraz-oliveira
Copy link
Contributor Author

@bjorng: I squashed most of the previous stuff (now force-pushed), then simplified the test suite.

Copy link
Contributor

@bjorng bjorng left a comment

Choose a reason for hiding this comment

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

Thanks for the update. I have some more suggestions for changes. When you have done those changes, please squash to a single commit.

lib/stdlib/test/erl_lint_SUITE.erl Outdated Show resolved Hide resolved
lib/stdlib/test/erl_lint_SUITE.erl Outdated Show resolved Hide resolved
lib/stdlib/test/erl_lint_SUITE.erl Outdated Show resolved Hide resolved
@paulo-ferraz-oliveira
Copy link
Contributor Author

@bjorng, I think I've handled all your requests. I squashed all previous commits. Thanks for your help.

@bjorng bjorng added the testing currently being tested, tag is used by OTP internal CI label Dec 10, 2020
@bjorng
Copy link
Contributor

bjorng commented Dec 10, 2020

Thanks! Added to our daily builds.

@bjorng bjorng merged commit c759940 into erlang:master Dec 11, 2020
@bjorng
Copy link
Contributor

bjorng commented Dec 11, 2020

Thanks for the pull request!

@paulo-ferraz-oliveira paulo-ferraz-oliveira deleted the doc/hidden_compiler_options branch January 5, 2021 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement team:VM Assigned to OTP team VM testing currently being tested, tag is used by OTP internal CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants