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

Make case classes final #69

Merged
merged 1 commit into from
Oct 24, 2024
Merged

Conversation

mdedetrich
Copy link
Contributor

Its idiomatic for case classes to be final since users overriding the case class can cause unexpected behaviour

@@ -3,4 +3,4 @@ package com.github.sbt.sbom
import org.cyclonedx.Version
import sbt.Configuration

case class BomExtractorParams(schemaVersion: Version, configuration: Configuration)
final case class BomExtractorParams(schemaVersion: Version, configuration: Configuration)
Copy link
Member

@eed3si9n eed3si9n Oct 18, 2024

Choose a reason for hiding this comment

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

Idiomatically all case classes are assumed to be final. Any attempt to extend them is non-idiomatic, and asking for trouble. For sbt plugins I don't think putting this guardrail is necessary.

Copy link
Contributor

@raboof raboof left a comment

Choose a reason for hiding this comment

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

I don't think it is universally 'idiomatic' to explicitly mark case classes as final, and I personally find that in this case it adds noise for questionable benefit, but if you feel strongly I'm OK with it.

@mdedetrich mdedetrich force-pushed the make-case-classes-final branch from e256c70 to 7bd9638 Compare October 24, 2024 08:05
@mdedetrich
Copy link
Contributor Author

mdedetrich commented Oct 24, 2024

I don't think it is universally 'idiomatic' to explicitly mark case classes as final, and I personally find that in this case it adds noise for questionable benefit, but if you feel strongly I'm OK with it.

Yeah tbh this is something that should be solved by a linting tool, there was a big discussion somewhere that case class should be final by default and the only argument against it was some niche Scala2 scalac usecase that was given by Martin.

So you are right that its not idiomatic to mark case class as final (although a lot of "serious" projects in open source Scala do this) mainly because of the annoyance but treating case class as final is almost universally idiomatic for reasons given before.

@raboof
Copy link
Contributor

raboof commented Oct 24, 2024

treating case class as final is almost universally idiomatic

Yes, no disagreement there :)

@mdedetrich mdedetrich merged commit 40c31f1 into sbt:main Oct 24, 2024
10 checks passed
@mdedetrich mdedetrich deleted the make-case-classes-final branch October 24, 2024 08:12
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.

3 participants