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

feature: Add static factory methods for CtExtendedModifier #4202

Merged
merged 2 commits into from
Oct 12, 2021

Conversation

SirYwell
Copy link
Collaborator

@SirYwell SirYwell commented Oct 4, 2021

This adds two methods to CtExtendedModifier that allow to create instances without having to pass boolean values.

I deprecated the constructor where the implicitness defaulted to false, as the result is less obvious compared to the named method.

I also replaced the usages of the deprecated constructor and the usages of the other constructor where the second parameter was a boolean literal.

The usage as API might be more relevant with methods to directly add and remove extended modifiers, see #4136 for context.

@SirYwell SirYwell changed the title wip: feature: Add static factory methods for CtExtendedModifier review: feature: Add static factory methods for CtExtendedModifier Oct 4, 2021
* @param kind the kind of this modifier.
* @deprecated use {@link #explicit(ModifierKind)} to create an explicit modifier.
*/
@Deprecated
Copy link
Collaborator

Choose a reason for hiding this comment

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

@monperrus
Copy link
Collaborator

LGTM. I would not deprecate the existing constructor to maximize backward compatibility (we love backward compatibility in Spoon). It just assumes a default implicitness, which we document and that's fine.

@slarse
Copy link
Collaborator

slarse commented Oct 8, 2021

I would not deprecate the existing constructor to maximize backward compatibility (we love backward compatibility in Spoon). It just assumes a default implicitness, which we document and that's fine.

I disagree. By providing the explicit static method, we provide two ways of achieving the same thing, namely creating an explicit modifier. As a general rule, I think it's best to provide one way of doing something, or at least one canonical way. By deprecating the constructor, we clearly show the preferred way of doing it, without actually affecting backward compatibility (we never need to act on the deprecation, not really).

Just my two cents, I'm in favor of the deprecation given that these static methods should be added at all (which I don't have a strong opinion about).

EDIT: Actually, by my logic, both existing constructors should be deprecated.

@SirYwell
Copy link
Collaborator Author

SirYwell commented Oct 8, 2021

we never need to act on the deprecation

@Deprecated got a forRemoval value which defaults to false, so if we want to remove it in future, we could use that - but I don't see a reason for that either.

EDIT: Actually, by my logic, both existing constructors should be deprecated.

The other constructor is currently used in places where the implicitness is given a boolean variable (or parameter). This should still be possible without having deprecation warnings. But if we have a static factory method for that too, we could deprecate it I think.

@slarse
Copy link
Collaborator

slarse commented Oct 9, 2021

we never need to act on the deprecation

@Deprecated got a forRemoval value which defaults to false, so if we want to remove it in future, we could use that - but I don't see a reason for that either.

I've never actually seen that used :). But it does further prove my point that deprecation does not mean "planned for removal". According to the Javadoc of @Deprecated (emphasis mine):

A program element annotated @deprecated is one that programmers are discouraged from using, typically because it is dangerous, or because a better alternative exists.

So I vote for deprecation of that constructor.

The other constructor is currently used in places where the implicitness is given a boolean variable (or parameter). This should still be possible without having deprecation warnings. But if we have a static factory method for that too, we could deprecate it I think.

I suppose that's fair, rewriting that code to use explicit and implicit static methods would be a bit awkward.

@SirYwell
Copy link
Collaborator Author

SirYwell commented Oct 9, 2021

I've never actually seen that used

I forgot to mention that this was added in Java 9, so that might be the reason

Copy link
Collaborator

@slarse slarse left a comment

Choose a reason for hiding this comment

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

To me this is ready for merge.

@monperrus Do you want to veto the deprecation? I don't feel super strongly about it so I'll let you decide, but I would vote for including it (as argued above).

@monperrus
Copy link
Collaborator

we never need to act on the deprecation

I love that. We use deprecation for signaling better alternatives (free added value for clients), but not for signaling removal (forced labor for clients).

Merging! Thanks a lot!

@monperrus monperrus changed the title review: feature: Add static factory methods for CtExtendedModifier feature: Add static factory methods for CtExtendedModifier Oct 12, 2021
@monperrus monperrus merged commit f6abeff into INRIA:master Oct 12, 2021
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