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

Disallow records as part of public API #4279

Merged
merged 4 commits into from
Dec 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,9 @@ Fabric API makes strong backwards compatibility guarantees, by which contributor
- If vanilla exposes optionals in return types, then returning an optional is fine.
- Avoid requiring the user to cast to a subtype if possible.
- Adding methods to vanilla types can be done via interface injection.
- Avoid exposing java `record`s as public API.
- Records expose more than is necessary for most APIs, which makes them difficult to evolve.
- Prefer to expose an interface that is implemented by an impl record.

### API design patterns to consider

Expand Down
5 changes: 5 additions & 0 deletions checkstyle.xml
Original file line number Diff line number Diff line change
Expand Up @@ -208,5 +208,10 @@
<module name="MatchXpath">
<property name="query" value="//VARIABLE_DEF[./TYPE/IDENT[@text='var'] and not(./ASSIGN/EXPR/LITERAL_NEW)]"/>
</module>

<!-- Prevent public records in API packages -->
<module name="MatchXpath">
<property name="query" value="//RECORD_DEF[./MODIFIERS/LITERAL_PUBLIC][//PACKAGE_DEF//DOT/IDENT[@text='api']]"/>
</module>
Copy link
Contributor

Choose a reason for hiding this comment

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

You might want to add a custom error message here, as I don't think this will generate failures that are easy to understand if you don't know about the record rule. I don't know much about configuring checkstyle but custom messages seem possible: https://stackoverflow.com/a/18252466

</module>
</module>
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@

package net.fabricmc.fabric.api.transfer.v1.storage.base;

//CHECKSTYLE.OFF: MatchXpath
Copy link
Contributor

Choose a reason for hiding this comment

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

These MatchXpath checks could have different IDs so that they can be suppressed individually

Copy link
Member Author

Choose a reason for hiding this comment

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

Im not aware of a way to do this

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried that, it doesnt seem to work.

Screenshot 2024-12-06 at 10 38 49

I also cannot get the custom messages to work.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh well, I guess leave it then


/**
* An immutable object storing both a resource and an amount, provided for convenience.
* @param <T> The type of the stored resource.
Expand Down