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

New elvis_style rule: no_init_lists #367

Merged
merged 32 commits into from
Oct 27, 2024
Merged

Conversation

bormilan
Copy link
Contributor

@bormilan bormilan commented Oct 7, 2024

Description

A brief description of your changes.

Closes #329;.


EDIT: I made a checklist based on @elbrujohalcon 's comment to to make the progress traceable.

  • has init([|]) -> …: Should fail.

  • has init(A = [1,2,3]) -> …: Should fail

  • has init/1 implemented with multiple clauses, all of them expecting a list: Should fail.

  • - [ ] A module that has two behaviours and has init([]) -> should fail

  • has init/1 implemented with multiple clauses, one of them not being a list: Should pass, even if other clauses expect a list.

  • has init/1 implemented using a map, but init/2 implemented using a list in one of the arguments: Should pass.

  • has init/0 implemented using a tuple, but also init/0: Should pass.

  • A module that doesn't implement a behaviour and has init([1,2,3]) -> …: Should pass.

  • A module that has a behaviour that is not in the list of the rule's config -> should pass(with default config)

@bormilan
Copy link
Contributor Author

bormilan commented Oct 7, 2024

Do we want to check that the module implements a gen_* behavior, or should we check every module like now?
Oh, and I want to check if this rule can work on .beam files.

Copy link
Member

@elbrujohalcon elbrujohalcon left a comment

Choose a reason for hiding this comment

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

This needs more tests, specifically:

  • A module that implements gen_server or gen_statem and…
    • has init([_|_]) -> …: Should fail.
    • has init(A = [1,2,3]) -> …: Should fail
    • has init/1 implemented with multiple clauses, all of them expecting a list: Should fail.
    • has init/1 implemented with multiple clauses, one of them not being a list: Should pass, even if other clauses expect a list.
    • has init/1 implemented using a map, but init/2 implemented using a list in one of the arguments: Should pass.
    • has init/0 implemented using a tuple, but also init/0: Should pass.
  • A module that doesn't implement a behaviour and has init([1,2,3]) -> …: Should pass.

RULES.md Outdated Show resolved Hide resolved
doc_rules/elvis_style/no_init_lists.md Outdated Show resolved Hide resolved
doc_rules/elvis_style/no_init_lists.md Outdated Show resolved Hide resolved
src/elvis_style.erl Outdated Show resolved Hide resolved
test/examples/fail_verify_no_init_lists.erl Outdated Show resolved Hide resolved
Copy link
Collaborator

@paulo-ferraz-oliveira paulo-ferraz-oliveira left a comment

Choose a reason for hiding this comment

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

You're on the right path here ❤️ Just a few more tweaks...

@paulo-ferraz-oliveira paulo-ferraz-oliveira changed the title No init lists New elvis_style rule: no_init_lists Oct 8, 2024
@paulo-ferraz-oliveira
Copy link
Collaborator

I tweaked the PR title since this'll go directly into the generated release Changelog.

@bormilan
Copy link
Contributor Author

bormilan commented Oct 10, 2024

Thanks for the comments, I will solve all of them if I have some time. ❤️

Copy link
Member

@elbrujohalcon elbrujohalcon left a comment

Choose a reason for hiding this comment

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

Still missing some changes, but I just wanted to add feedback about the recent additions.

src/elvis_style.erl Outdated Show resolved Hide resolved
src/elvis_style.erl Outdated Show resolved Hide resolved
@bormilan
Copy link
Contributor Author

I still have some work to do, but it keeps getting better.

@elbrujohalcon
Copy link
Member

@bormilan do you mind merging main into your branch, please?

Copy link
Member

@elbrujohalcon elbrujohalcon left a comment

Choose a reason for hiding this comment

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

A few small suggestions, but overall great progress @bormilan 👍🏻

src/elvis_style.erl Outdated Show resolved Hide resolved
src/elvis_style.erl Outdated Show resolved Hide resolved
src/elvis_style.erl Outdated Show resolved Hide resolved
src/elvis_style.erl Outdated Show resolved Hide resolved
src/elvis_style.erl Outdated Show resolved Hide resolved
src/elvis_style.erl Outdated Show resolved Hide resolved
src/elvis_style.erl Outdated Show resolved Hide resolved
Copy link
Member

@elbrujohalcon elbrujohalcon left a comment

Choose a reason for hiding this comment

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

One more test (or maybe two, if you want a fail and a pass), @bormilan:

  • A module that implements gen_server AND another behaviour…
-module(fail…).

-behaviour(gen_server).
-behaviour(another_behaviour).

-export([init/1]).

init([]) -> {error, "should not be a list"}.

Oh! And another test! A module that implements a behaviour that's not gen_server, nor gen_statem

-module(pass…).

-behaviour(another_behaviour).

-export([init/1]).

init([]) -> {error, "can be a list"}.

(This one should pass with default options, but it should fail if you adjust the behaviours list by adding another_behaviour to it).

@bormilan
Copy link
Contributor Author

One more test (or maybe two, if you want a fail and a pass), @bormilan:

  • A module that implements gen_server AND another behaviour…
-module(fail…).

-behaviour(gen_server).
-behaviour(another_behaviour).

-export([init/1]).

init([]) -> {error, "should not be a list"}.

Yes! I like making more and more tests to feel that my code is 100% good.

@elbrujohalcon
Copy link
Member

Yes! I like making more and more tests to feel that my code is 100% good.

HA! I just saw that you were matching against a single [BehaviourNode] in your code… and I wondered "what would happen if we have multiple ones?"… so… we have to test that, right?

@bormilan
Copy link
Contributor Author

Yes! I like making more and more tests to feel that my code is 100% good.

HA! I just saw that you were matching against a single [BehaviourNode] in your code… and I wondered "what would happen if we have multiple ones?"… so… we have to test that, right?

You're right. I'm working on the new logic right now, so it's the perfect time to add new tests and consider all the possible barriers.

@bormilan
Copy link
Contributor Author

The only test that is not working now is the one with the init([]) because its attribute type is nil. I don't know how I will solve this, it may need some extra magic. Or is it a bug in katana_code?

@elbrujohalcon
Copy link
Member

The only test that is not working now is the one with the init([]) because its attribute type is nil. I don't know how I will solve this, it may need some extra magic. Or is it a bug in katana_code?

Isn't it as simple as lists:member(ktn_code:type(Attr), [cons, nil])?

I think nil is precisely [], which is fine.

src/elvis_style.erl Outdated Show resolved Hide resolved
src/elvis_style.erl Outdated Show resolved Hide resolved
@bormilan
Copy link
Contributor Author

The only test that is not working now is the one with the init([]) because its attribute type is nil. I don't know how I will solve this, it may need some extra magic. Or is it a bug in katana_code?

Isn't it as simple as lists:member(ktn_code:type(Attr), [cons, nil])?

I think nil is precisely [], which is fine.

Oh! Okay, that's nice, I started to dig into the code of katana, thanks for the clarification.

Copy link
Member

@elbrujohalcon elbrujohalcon left a comment

Choose a reason for hiding this comment

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

We're almost there, @bormilan … really really close to the final version.

src/elvis_style.erl Outdated Show resolved Hide resolved
src/elvis_style.erl Outdated Show resolved Hide resolved
src/elvis_style.erl Outdated Show resolved Hide resolved
src/elvis_style.erl Outdated Show resolved Hide resolved
Copy link
Member

@elbrujohalcon elbrujohalcon left a comment

Choose a reason for hiding this comment

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

Excellent job, @bormilan !! Only one more change.

Untitled Outdated Show resolved Hide resolved
@elbrujohalcon elbrujohalcon merged commit 979a586 into inaka:main Oct 27, 2024
7 checks passed
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.

Rule: Do not use lists on start_link/3,4 and init/1
3 participants