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

Prevents jvm incompat dataclass constructors with value class params #329

Merged

Conversation

jackcviers
Copy link
Contributor

@jackcviers jackcviers commented Aug 17, 2023

@jackcviers jackcviers force-pushed the 8678h9apz-inline-value-class-lint branch 2 times, most recently from f252e0e to 7116c3f Compare August 17, 2023 05:13
* To prevent issues like xebia-functional#275, dataclasses must not use parameters
that are value classes without a `JvmStatic` factory method in their
companion object.
@jackcviers jackcviers force-pushed the 8678h9apz-inline-value-class-lint branch from 7116c3f to c92c7e9 Compare August 17, 2023 05:34
@jackcviers jackcviers changed the title Creates lint for dataclasses that use value class parameters requiring static apply methods Prevents jvm incompat dataclass constructors with value class params Aug 17, 2023
Copy link
Contributor

@raulraja raulraja left a comment

Choose a reason for hiding this comment

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

Thank you, Jack!

After looking over the files, it seems at first sight all issues here are related to the use of the value class.
If that is the case we need a rule to disallow using value classes.
I think the rule for static companion factories is unnecessary, and in main, we now have instances of multiple cases where the Prompt class, a data class, is called from Java to any of its constructors.

Can we have a single rule to disallow value instead of 2 rules?

@jackcviers
Copy link
Contributor Author

Thank you, Jack!

After looking over the files, it seems at first sight all issues here are related to the use of the value class.
If that is the case we need a rule to disallow using value classes.
I think the rule for static companion factories is unnecessary, and in main, we now have instances of multiple cases where the Prompt class, a data class, is called from Java to any of its constructors.

Can we have a single rule to disallow value instead of 2 rules?

We can. It would also result in a change to RequestConfig that would not be source compatible, if that's acceptable. My intention with these lints is to be as non-invasive as possible to the declared api to avoid disruption of the current pace of development.

If we don't mind me changing the signature of RequestConfig and potentially call sites (I think 11, currently), the current two Lint riles can be collapsed to a single rule disallowin value classes in common and jvm source sets. I can address this very quickly, just give me a 👍 on this comment.

It should be noted, if the concern is additional rules, we will need additional rules anyway:

Ok. So action item out of this is --

Lint rule for no value classes in common/jvm.
Lint rule for jvm overloads with constructor args on data classes.

In addition we need rules for avoiding the creation of public common and jvm apis relying on kotlin annotations, which cannot be called as annotations from scala. We can achieve the same effect by declaring jvm annotations in a Java class for the jvm, and platform specific annotations and processors can be declared for the other multiplatform source sets. Currently, only Description.kt and JsonSchema.kt and the @Description (17 call sites) will require changes.

Finally, I intend to ensure all platforms have equivalent examples, which may reveal additional required lint rules.

@raulraja
Copy link
Contributor

We can make those changes to RequestConfig. We are currently ongoing heavy API changes to simplify the library.
The reality is that having used value classes has introduced more issues than benefits, and the code we have to add to work around them is higher than the benefit of saving an allocation.

raulraja
raulraja previously approved these changes Aug 17, 2023
Copy link
Contributor

@raulraja raulraja left a comment

Choose a reason for hiding this comment

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

thank you Jack!

@raulraja raulraja merged commit 9f673f6 into xebia-functional:main Aug 18, 2023
5 checks passed
@jackcviers jackcviers deleted the 8678h9apz-inline-value-class-lint branch August 18, 2023 13:10
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.

None yet

3 participants