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

Adds Elixir linter #23

Merged
merged 1 commit into from
Feb 8, 2017
Merged

Adds Elixir linter #23

merged 1 commit into from
Feb 8, 2017

Conversation

frm
Copy link
Contributor

@frm frm commented Oct 31, 2016

Why:

  • With the growing number of projects using Elixir, we should try to follow the
    same style guides across all of them.

This change addresses the need by:

  • Adding a .credo.exs file containing a sample configuration.

Notes:

  • This PR shouldn't be considered a definitive proposal, but an initial,
    open-to-discussion proposal.
  • These rules haven't been battle tested, but they are the ones being used in
    atmos.
  • The sample configuration was obtained from the .credo.exs file of the credo
    repository
    . It contains
    all the available linting options.
  • I tried to base myself on the Ruby linter, but for the time being, it values
    consistency in the current project as opposed to an overall rule for all
    projects.
  • I, sadly, still haven't found any rule that enforces trailing commas over multiline
    maps, function declarations and arrays.

@frm
Copy link
Contributor Author

frm commented Feb 6, 2017

A small update on this: I've been trying out different configs. This is the one that seems to do well with a community style guide with some small tweaks to include some of our practices.

color: true,

checks: [
{Credo.Check.Design.TagTODO, exit_status: 0},
Copy link
Member

Choose a reason for hiding this comment

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

What does this do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm actually on the fence on this one and would like a second opinion. TODO comments, by default, make mix credo exit with status code 2 and the CI fail. Having exit_status: 0 still makes the check but the CI doesn't fail. Which means when you run mix credo it's going to output a warning and remind you. It's not considered a failed check.

Copy link
Member

Choose a reason for hiding this comment

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

Well, someone said that a TODO is a fuck you to the other developers. That being said, I myself have added them from time to time as a reminder to comeback to it.

I don't have a strong opinion either way, but do think that breaking the build because of it is a tad too much for my liking. So in principle I'm ok with this.


checks: [
{Credo.Check.Design.TagTODO, exit_status: 0},
{Credo.Check.Readability.ModuleDoc, false},
Copy link
Member

Choose a reason for hiding this comment

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

What are these about?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By default, credo requires a @moduledoc with a given model's description. This setting disables that check.

Copy link
Member

Choose a reason for hiding this comment

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

That's something that Elixir tries to enforce as a good practice as well, and I like it, but for public projects, not for internal apps. I'm thinking if it would make sense to have an OSS linter and an internal linter. /cc @naps62

Copy link
Member

Choose a reason for hiding this comment

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

If this is the only rule, is it really worth it to have two linters?

Copy link
Member

Choose a reason for hiding this comment

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

Probably not. We can do with what we're using now for other technologies which is tests as documentation and forego documentation in the modules.

checks: [
{Credo.Check.Design.TagTODO, exit_status: 0},
{Credo.Check.Readability.ModuleDoc, false},
{Credo.Check.Readability.Specs, false},
Copy link
Member

Choose a reason for hiding this comment

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

What is this for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Disables the enforce of a typespec (@spec) when overriding behaviours. An example would be to create a mix app with a module that contains use Application. The required start function, without disabling that check, needs to have a @spec specification.

It seemed odd to me, given the name of the check, but it doesn't seem to enforce the presence of a typespec in any other function. I couldn't find documentation for that as well.

Why:

* With the growing number of projects using Elixir, we should try to follow the
same style guides across all of them.

This change addresses the need by:

* Adding a `.credo.exs` file containing a sample configuration.
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.

3 participants