Skip to content
This repository has been archived by the owner on May 12, 2018. It is now read-only.

Add erl_first_files in eunit_first_files and qc_first_files #539

Merged
merged 2 commits into from
Sep 21, 2015

Conversation

platinumthinker
Copy link
Contributor

Fix compile order for tests

@ghost
Copy link

ghost commented Aug 26, 2015

This looks reasonable and needs a test before merge, but why did you restrict it to "qc" and "eunit"?

@platinumthinker
Copy link
Contributor Author

I saw this behaviour in tests. I add erl_first_files in all first_files..
I think about general first_files instead of separate.

@ghost
Copy link

ghost commented Aug 27, 2015

You can skip the list_to_atom call and just use erl_first_files in the new version.

Regarding tests, it makes sense to check the new behavior explicitly in a test, maybe as follows:

  1. eunit with first rebar.config, which does not have erl_first_files, and expect build failure
  2. clean
  3. eunit with second rebar.config, which has erl_first_files, and expect build+eunit success

@ghost
Copy link

ghost commented Sep 11, 2015

ping?

@platinumthinker
Copy link
Contributor Author

I have been on vacation. I am back now and continue to work on this.

@platinumthinker
Copy link
Contributor Author

ping?

@ghost
Copy link

ghost commented Sep 17, 2015

  1. The unneeded list_to_atom("erl_first_files") is still there.
  2. You're not testing the case where the test fails without erl_first_files. However, I don't think you can test that, because erlc_compiler detects a parse_transform compiler directive and automatically adds it as a dependency. I'm not sure how to test the failure case.

@platinumthinker
Copy link
Contributor Author

I agree with you on second position. First thing is fixed.

@ghost
Copy link

ghost commented Sep 18, 2015

You should simply use the atom erl_first_files. No variable needed.

@ghost
Copy link

ghost commented Sep 18, 2015

There's no need to quote erl_first_files.

@platinumthinker
Copy link
Contributor Author

This true, but not mandatory.
Example in this file:
41: -type erlc_info_v() :: {digraph:vertex(), term()} | 'false'.
210: {platform_define, "R13", 'old_inets'}]},

@ghost
Copy link

ghost commented Sep 18, 2015

Yes, but (1) in type specs it's customary to quote atoms and (2) we don't quote atoms in any other rebar_config call, which makes this case break the consistency for no reason.

@platinumthinker
Copy link
Contributor Author

okay

@ghost
Copy link

ghost commented Sep 19, 2015

Thanks. +1, once you've squashed the fixups into the original commit.

@platinumthinker
Copy link
Contributor Author

Squashed is done

ferd added a commit that referenced this pull request Sep 21, 2015
Add erl_first_files in eunit_first_files and qc_first_files
@ferd ferd merged commit 5d97cb7 into rebar:master Sep 21, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants