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 Credo #56

Merged
merged 13 commits into from
Apr 24, 2021
Merged

Add Credo #56

merged 13 commits into from
Apr 24, 2021

Conversation

angelikatyborska
Copy link
Member

@angelikatyborska angelikatyborska commented Apr 23, 2021

This PR adds credo and fixes all problems reported by it. It also adds the credo check to the CI.

I'm also sneaking in .tool-versions, the same as the test runner and representer have.

@angelikatyborska angelikatyborska marked this pull request as ready for review April 23, 2021 08:01
Comment on lines +38 to +43
# exclude exercise analysis because of snippets in `feature do form do...`
# they can be unusual, e.g. `_ignore || _ignore`
"lib/elixir_analyzer/test_suite/",
# exclude exercise analysis test because of example solutions used in tests
# those mimic student solutions and might be "bad code" on purpose
"test/elixir_analyzer/test_suite/"
Copy link
Member Author

@angelikatyborska angelikatyborska Apr 23, 2021

Choose a reason for hiding this comment

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

This is the only change in this config that I made, compared to the default config.

@@ -23,6 +23,7 @@ defmodule ElixirAnalyzer.ExerciseTest do
#

defmacro __before_compile__(env) do
# credo:disable-for-previous-line Credo.Check.Refactor.CyclomaticComplexity
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the only place where I was afraid of splitting this code to reduce cyclomatic complexity, so I took the easy way out...

Copy link
Contributor

@neenjaw neenjaw left a comment

Choose a reason for hiding this comment

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

Looks good, forgot how opinionated credo is about cond vs if, but I'm good with it

Comment on lines +1 to +2
elixir 1.11.3-otp-23
erlang 23.2.6
Copy link
Contributor

Choose a reason for hiding this comment

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

👍🏻 Looks good!

@angelikatyborska angelikatyborska merged commit 324c52e into main Apr 24, 2021
@angelikatyborska angelikatyborska deleted the add-credo branch April 24, 2021 09:07
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.

2 participants