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 Credo to Elixir test suite #1769

Merged
merged 6 commits into from Dec 7, 2018

Conversation

Projects
None yet
5 participants
@dottorblaster
Copy link
Contributor

dottorblaster commented Nov 28, 2018

Overview

This PR adds Credo as the static code analysis tool of choice for the Elixir test suite.

Testing recommendations

Just issue make elixir-credo first, then make elixir

Related Issues or Pull Requests

#1756

Checklist

  • Code is written and works correctly;
  • Changes are covered by tests;
  • Documentation reflects the changes;

@dottorblaster dottorblaster changed the title test: add credo to Elixir tests mix.exs Add credo to Elixir tests mix.exs Nov 28, 2018

@dottorblaster dottorblaster changed the title Add credo to Elixir tests mix.exs Add Credo to Elixir test suite Nov 28, 2018

@dottorblaster

This comment has been minimized.

Copy link
Contributor

dottorblaster commented Nov 28, 2018

Here is a rationale of this PR.

I added Credo to the Mix project, then I disabled some "not so useful" checks. They are:

  • Credo.Check.Refactor.CyclomaticComplexity
  • Credo.Check.Refactor.LongQuoteBlocks
  • Credo.Check.Refactor.Nesting

Then, I added Credo to the Makefile and Makefile.win with a proper goal, and I added that goal to make elixir. This way, make elixir formats the code according to #1767, then executes the elixir-credo goal erroring on basically everything.

The only check I maintained but I changed its exit code to 0 is Credo.Check.Design.TagTODO, because I want to be warned about TODOs and monitor them, but I still want to be able to use them as reminders.

@wohali I think your eyes could be useful here 🙂

@wohali

wohali approved these changes Nov 28, 2018

Copy link
Member

wohali left a comment

Overall this looks good, a few minor comments.

I'm eager to get ahead of any code style issues, as our Erlang code base uses a style that presently doesn't have a formal checker (@davisp is working on one...someday...)

Perhaps it's time for me to add black for our Python code, too... :)

+1 after addressing my small comments

Show resolved Hide resolved Makefile
@@ -1,3 +1,5 @@
# credo:disable-for-this-file Credo.Check.Consistency.ParameterPatternMatching

This comment has been minimized.

@wohali

wohali Nov 28, 2018

Member

Why wouldn't we disable this globally, or fix the problem in this one file? Keep in mind I am new to Elixir, but not to linting and formatting...exceptions in a single file are usually bad news.

This comment has been minimized.

@davisp

davisp Nov 28, 2018

Member

Totally agree with @wohali on this one lets avoid file/line specific exemptions unless absolutely necessary.

This comment has been minimized.

@wohali

wohali Dec 6, 2018

Member

@dottorblaster This still needs addressing before I can approve this PR.

This comment has been minimized.

@dottorblaster

dottorblaster Dec 7, 2018

Contributor

Done! Sorry, I missed it 🙇

@jaydoane

This comment has been minimized.

Copy link
Contributor

jaydoane commented Nov 28, 2018

I have a general question about max line length. This commit explicitly sets the max line length to 120, up from its default of 96. For Erlang code, the current consensus (and also @davisp's emelio) appears to be 80, for which there is ample precedent. Because it forces you to write more concise code, and it’s great being able to have two code windows open side by side (on a 15" laptop), I hope we also adopt 80 for Elixir code. But I'm curious to what others think it should be.

@davisp
Copy link
Member

davisp left a comment

I am totally on board with adding this. Though I also agree with Jay that the 120 line length seems awfully long. I personally prefer 80 but if there's a community consensus around a specific width then I'd of course defer to the existing language consensus.

@@ -1,3 +1,5 @@
# credo:disable-for-this-file Credo.Check.Consistency.ParameterPatternMatching

This comment has been minimized.

@davisp

davisp Nov 28, 2018

Member

Totally agree with @wohali on this one lets avoid file/line specific exemptions unless absolutely necessary.

@wohali

This comment has been minimized.

Copy link
Member

wohali commented Nov 28, 2018

@jaydoane After having been an 80-column adherent for 30 years now (I started on VT-52 terminals connected to a PDP-11/34A), I'm growing more in love with slightly longer line length.

I want to propose Black for our Python code, which adopts an 88-character limit. They have a researched answer as to why 90-ish is probably better than 80, complete with YouTube video.

Over in apache/couchdb-documentation, due to limitations of the ReStructuredText parser and some very long URL references we can't line wrap without breaking the syntax, we've had to recently relax line length from 80 to 90 for similar reasons.

@davisp @jaydoane how would you feel about 90?

@wohali

This comment has been minimized.

Copy link
Member

wohali commented Nov 28, 2018

@jaydoane also, as to your comment on "two code windows open side by side (on a 15" laptop)" is highly variable. I use a 15" laptop and often get 3 or 4 code windows open side by side, with >80 characters each. Resolution and font size (and your own visual acuity!) enter into it a lot :)

@jaydoane

This comment has been minimized.

Copy link
Contributor

jaydoane commented Nov 29, 2018

@wohali I much prefer 90 to 120. And you are right about numerous variables factoring into how many characters are comfortable to squeeze on a screen and read easily over long time periods.

Hmm, I'm not sure I find it more readable with all the additional vertical space it uses in some places going from 120 to 90.

@wohali
Copy link
Member

wohali left a comment

Two major changes that need to be addressed, see comments.

Show resolved Hide resolved test/elixir/.credo.exs Outdated

@wohali wohali referenced this pull request Nov 29, 2018

Merged

Format and check all Python code using python black #1776

3 of 3 tasks complete
@wohali

This comment has been minimized.

Copy link
Member

wohali commented Nov 29, 2018

@jaydoane Hrm, I don't mind it...

@wohali
Copy link
Member

wohali left a comment

Another (to-me) mandatory change occurred to me as I was working on my Python code formatting PR.

Show resolved Hide resolved Makefile
@dottorblaster

This comment has been minimized.

Copy link
Contributor

dottorblaster commented Nov 29, 2018

Some comments on the above discussion:

  • Wow, you were great guys, that was such a beautiful debate 😬
  • I enforced max line length to 120 because our formatter enforces the coding style to 120-chars lines. I'm gonna take it to 90 in Credo, and of course I'm updating the .formatter.exs as well.
  • Also, I love the 120 limit, and the community does. But if we want to enforce a 90 columns limit, that's the beautiful point of having a tool that you can configure 😹
  • About disabling Credo.Check.Consistency.ParameterPatternMatching and the long quotes one: Consistency.ParameterPatternMatching is ok, but I think: 1) that we need a long quote block for our test helper 2) that long quote blocks are smells and we should limit that smell to a known place.

Enforcing a 90 columns format will cause almost all of our tests to be reformatted. @wohali et al. do you want me to go further or do we want to do that in another subsequent PR?

@davisp

This comment has been minimized.

Copy link
Member

davisp commented Nov 29, 2018

Reformatting all the lines that require it to pass a make check invocation is totally fine. Just keep it to an isolated commit and then make sure that make check fails after the fact so that we're not re-introducing breaking things after the fact.

@dottorblaster

This comment has been minimized.

Copy link
Contributor

dottorblaster commented Nov 29, 2018

@davisp thanks! I'm going further then

@van-mronov van-mronov referenced this pull request Nov 29, 2018

Merged

Refactor elixir test suite initialization #1779

0 of 3 tasks complete
@van-mronov

This comment has been minimized.

Copy link
Contributor

van-mronov commented Nov 29, 2018

Hi all!

Not long ago we also added checking elixir code format with mix format before running elixir test suite. Since line length is more code formatting issue rather than some logic error, like not used local var or something like that, I believe it's better to address this to mix format.

Here is formatter options, :line_length exactly what we need. We just need to add this in our .formatter.exs wuth reasonable value. In that case make elixir if new changes have incorrect line length. And developer just need to run mix format to reformat his changes.

In that case mix credo have to address only logic code errors.

@van-mronov

This comment has been minimized.

Copy link
Contributor

van-mronov commented Nov 29, 2018

@dottorblaster sorry, looks like you are going to add line_length in formatter.

@dottorblaster

This comment has been minimized.

Copy link
Contributor

dottorblaster commented Nov 29, 2018

@van-mronov yeah line_length is already inside the .formatter.exs and I'm gonna enforce that to 90 as I said before, no problem

@dottorblaster

This comment has been minimized.

Copy link
Contributor

dottorblaster commented Nov 29, 2018

@wohali @davisp @jaydoane @van-mronov I added two more commits for that, gonna edit the Makefile soon, can you provide me feedbacks?

@van-mronov I still prefer to maintain max length checks in Credo too because it's more developer friendly. I enforced the value formatter-side as well.

@dottorblaster dottorblaster force-pushed the dottorblaster:integrate-credo-elixir branch 3 times, most recently from c8283b5 to 5019b6c Nov 30, 2018

@dottorblaster dottorblaster force-pushed the dottorblaster:integrate-credo-elixir branch from 5019b6c to 91a58cc Dec 2, 2018

@dottorblaster

This comment has been minimized.

Copy link
Contributor

dottorblaster commented Dec 2, 2018

@wohali just rebased this branch against the recent devclean appending happened in master on Makefiles 👍

@dottorblaster dottorblaster force-pushed the dottorblaster:integrate-credo-elixir branch from 0fd651b to 6886959 Dec 7, 2018

@dottorblaster dottorblaster force-pushed the dottorblaster:integrate-credo-elixir branch from 6886959 to 0e2a0d8 Dec 7, 2018

@wohali

wohali approved these changes Dec 7, 2018

@wohali

This comment has been minimized.

Copy link
Member

wohali commented Dec 7, 2018

+1 LGTM

@wohali wohali merged commit f9354ea into apache:master Dec 7, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment