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

change(export_used_types): don't warn on behaviour callbacks #371

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

bormilan
Copy link
Contributor

@bormilan bormilan commented Nov 5, 2024

Description

I implemented a way to get the callbacks for the behaviors in the module and subtract them from the function used in the rule's logic. This approach ensures that all callback types are ignored.

Additionally, there was a test that expected a warning from a module that no longer generates one. So, I removed that assertion, but I'm not sure if that was the correct decision.
Closes #308.

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.

I'm not sure about this fix.
This will still fail on the following scenarios:

I'm not rejecting the PR entirely, but I'll leave it in the hands of @paulo-ferraz-oliveira to decide if this improving is worth merging or not.

lists:map(fun(#{attrs := #{value := Behaviour}}) -> Behaviour end, Behaviours),

lists:append(
lists:map(fun(B) -> apply(B, behaviour_info, [callbacks]) end, BehaviourNames)).
Copy link
Member

Choose a reason for hiding this comment

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

A try…catch is needed here, just in case someone introduces a typo (-behaviour(gen_srvr).) or uses a behaviour that's defined outside of OTP (-behaviour(my_behaviour).).

@bormilan
Copy link
Contributor Author

bormilan commented Nov 6, 2024

You are absolutely right. It was my best shot to solve this issue, until the issue erlang/otp#4847 will be solved.

I'm curious what do you think about it, or do you think that we should wait for the mentioned otp change request to be solved?

@elbrujohalcon
Copy link
Member

You are absolutely right. It was my best shot to solve this issue, until the issue erlang/otp#4847 will be solved.

I'm curious what do you think about it, or do you think that we should wait for the mentioned otp change request to be solved?

I think we should wait… but let's hear what @paulo-ferraz-oliveira has to say.

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.

There are instances where not exporting a type is valid, even if it's used in a spec.
2 participants