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

Conversation

modmuss50
Copy link
Member

@modmuss50 modmuss50 commented Dec 5, 2024

Request for comment

Prevent the use of public records in API classes as this has come up a few times in reviews so would like to get some proper feedback on this. Its only a minor thing, but happens enough I think we should enforce it.

Why

  • Hard to expand/refactor, e.g removing a component isnt possible
  • Adding a component can be error prone as you need to ensure that a suitable consturctor exists
  • Often they arent supposed to be constructed by the API consumer
  • Not as clear, e.g you cannot write documentation on each method

Alternative

The following example record in an API class

record Data(String example) { }

would become

interface Data {
  String example();
}

and the actual record lives in an impl package:

record DataImpl(String example) implements Data { }

Where it make a lot of sense to use a record e.g ResourceAmount we can opt allow it. If anyone has any thoughts (for or against) please let them be known in this PR.

@modmuss50 modmuss50 changed the title RFC: Disallow records as part of public API Disallow records as part of public API Dec 5, 2024
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
<!-- 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

@@ -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

@modmuss50 modmuss50 added merge me please Pull requests that are ready to merge small change labels Dec 6, 2024
@modmuss50 modmuss50 merged commit 5f43f98 into FabricMC:1.21.4 Dec 12, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me please Pull requests that are ready to merge small change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants