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

Add basic linting for .app file #2035

Merged
merged 5 commits into from
Apr 17, 2019
Merged

Add basic linting for .app file #2035

merged 5 commits into from
Apr 17, 2019

Conversation

ankhers
Copy link

@ankhers ankhers commented Mar 19, 2019

Resolves #980

This currently just checks for the existence of the description and applications
keys and that the applications list has kernel and stdlib in it.

If there are other things that could be linted, I would be happy to add them.

This currently just checks for the existence of the description and applications
keys and that the applications list has kernel and stdlib in it.
@ferd
Copy link
Collaborator

ferd commented Mar 19, 2019

Can you think of any way to test this so we can make sure linting works as expected? I can help add the tests if you need assistance as well.

@ankhers
Copy link
Author

ankhers commented Mar 19, 2019

I am not familiar enough with common test. Is there a smiliar function to ExUnit's capture_io/3? If not, would you be open to having something similar be written for this project as a test helper?

Other than that I am unable to think of something at the moment.

@ferd
Copy link
Collaborator

ferd commented Mar 20, 2019

There's a bunch of IO-capturing functions -- ct:capture_start/0, ct:capture_get/0-1 and ct:capture_stop/0, but the current test suites capture things through mocking with meck, something like:

%% override the logging module
meck:new(rebar_log, [no_link, passthrough]),
_ = rebar_test_utils:run_and_check(CTConfig, RebarConfig, ["compile"], return),
History = meck:history(rebar_log),
Warnings = [{Str, Args} || {_, {rebar_log, log, [warn, Str, Args]}, _} <- History],
Errors = [{Str, Args} || {_, {rebar_log, log, [error, Str, Args]}, _} <- History],
%% do some checking here

The rebar_test_utils stuff is documented in https://github.com/erlang/rebar3/blob/master/CONTRIBUTING.md#tests

But if you want to do something like create one or two sample OTP apps, I can transform them to tests as well.

@ankhers
Copy link
Author

ankhers commented Mar 20, 2019

I'm happy to write the tests, but should the test(s) go into the rebar_compile_SUITE or elsewhere?

@ferd
Copy link
Collaborator

ferd commented Mar 20, 2019

Yeah that should be fine. You may want to use the earliest command that can give the linting result for test execution speed (i.e. just 'get-deps' might be sufficient as a command if you are also testing deps in the linting), but the compile suite is fine.

@ankhers
Copy link
Author

ankhers commented Apr 4, 2019

Sorry for the long delay on this.

I plan on writing a couple more tests for the different warnings that are being output at the moment. I just wanted to first ask if this is the proper way to check that the warnings are being output, or if I should be doing something slightly different.

?WARN("~p is missing stdlib from applications list", [AppFile]);
true -> ok
end;
_ -> ?WARN("~p requires a list for applications key", [AppFile])
Copy link
Collaborator

Choose a reason for hiding this comment

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

"for applications value" maybe would be clearer

@ferd
Copy link
Collaborator

ferd commented Apr 4, 2019

That's a perfectly good approach to test the warnings.

@ankhers
Copy link
Author

ankhers commented Apr 16, 2019

After going through the test helpers, I think this is the best we can do while still using them. Because it writes out all the keys. I'm happy to write tests that just manually does the application setup if that is preferred.

@ferd ferd merged commit 45aaba2 into erlang:master Apr 17, 2019
@ferd
Copy link
Collaborator

ferd commented Apr 17, 2019

Thanks for the contribution!

@ankhers ankhers deleted the lint_app_file branch April 17, 2019 00:18
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.

Lint the *.app and/or *.app.src files
2 participants