Skip to content

Conversation

@indocomsoft
Copy link
Member

No description provided.

@indocomsoft indocomsoft requested a review from evansb April 19, 2018 07:47
@coveralls
Copy link

coveralls commented Apr 19, 2018

Pull Request Test Coverage Report for Build 100

  • 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.05%) to 96.154%

Totals Coverage Status
Change from base Build 89: -0.05%
Covered Lines: 150
Relevant Lines: 156

💛 - Coveralls

@evansb
Copy link
Contributor

evansb commented Apr 19, 2018

Sorry, can you justify your decision of using Dialyzer?

  1. It's very slow if you have large dependencies like Phoenix
  2. It's neither sound nor complete
  3. Having a good code coverage should suffice.

Furthermore it will cause more friction to the developers that have to learn/lookup the types.

@indocomsoft
Copy link
Member Author

Ah well, it was just a thought. I trust your judgment on this.

Although I've noticed some errors generated that might be legitimate errors like changeset(entity, params \\ :empty) with :empty instead of %{}, and some pattern matching in a case that is unreachable because of the pattern match in the parameter.

Maybe a better approach to run it once in a while just as a quick check and fix instead of imposing it on travis?

@evansb
Copy link
Contributor

evansb commented Apr 19, 2018 via email

@indocomsoft
Copy link
Member Author

Alright, I'll fix stuffs that dialyxir complains about that are actually legit in this PR. Maybe keep dialyxir deps so that someone can run it once in a while? But I'll remove it from travis.

@indocomsoft
Copy link
Member Author

@evansb bump

@evansb evansb merged commit 880c023 into master May 19, 2018
@indocomsoft indocomsoft deleted the dialyzer branch May 19, 2018 08:46
indocomsoft added a commit that referenced this pull request Aug 17, 2018
* Add dialyzer

* Fix dialyzer offenses

* Fix credo offense

* Add try/rescue in mix tasks

* Add ex_unit to dialyzer

* Add path to dialyzer

* Use travis matrix instead

* Test override travis env

* Fix dialyzer conn_case

* Remove travis dev env

* Remove dialyzer no_return

* Remove mix dialyzer from travis script
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.

4 participants