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

Fix #265: New Rule: export used types #273

Merged
merged 8 commits into from
Feb 28, 2023
Merged

Fix #265: New Rule: export used types #273

merged 8 commits into from
Feb 28, 2023

Conversation

maco
Copy link
Collaborator

@maco maco commented Feb 28, 2023

No description provided.

@elbrujohalcon elbrujohalcon added this to the 2.1.0 milestone Feb 28, 2023
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.

Great job! The only thing missing, from my perspective, is to make the rule run on beam files and test that.

RULES.md Outdated Show resolved Hide resolved
src/elvis_core.erl Show resolved Hide resolved
src/elvis_rulesets.erl Show resolved Hide resolved
doc_rules/elvis_style/export_used_types.md Show resolved Hide resolved
src/elvis_style.erl Outdated Show resolved Hide resolved
@paulo-ferraz-oliveira
Copy link
Collaborator

Should we mention, somewhere, that exported types can also be opaque, as a general guidance?

@erszcz
Copy link

erszcz commented Feb 28, 2023

@paulo-ferraz-oliveira

Should we mention, somewhere, that exported types can also be opaque, as a general guidance?

I'm not sure of the intention of the question as a bystander, but FWIW opaque types have to be exported to be useful at all. In the local module they're just types, i.e. no difference between -type and -opaque (i.e. you can pattern match and construct an opaque in the module that defines it without constraints), they only have special treatment when referred to as external types, but for that to be possible, they have to be exported.

@paulo-ferraz-oliveira
Copy link
Collaborator

but FWIW opaque types have to be exported to be useful at all

I know 😄, there's even a compiler warning if I'm not mistaken.

What I'm hinting at is that this rule will make consumers just go "Oh, I need to add an export_type here" and do it without thinking about the consequences of potentially exposing a non-opaque. If it was they're intention before, sure, the type should already be opaque and exposed; if it wasn't, now that it's being exposed there are consequences to it.

src/elvis_style.erl Outdated Show resolved Hide resolved
@maco maco requested a review from elbrujohalcon February 28, 2023 19:35
@elbrujohalcon
Copy link
Member

Once we have a new release, we should run it on OTP itself.
This rule in particular encapsulates something that people were asking for a while back.

@paulo-ferraz-oliveira
Copy link
Collaborator

Here's a pointer from the OTP team and exporting types: "Our policy is to be restrictive with exporting types. Only input and return types should be necessary to export." which correlates with the rule.

@maco maco merged commit 72df3ba into main Feb 28, 2023
@maco maco deleted the 265-export-used-types branch February 28, 2023 21:02
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.

4 participants