Skip to content

Conversation

@jglick
Copy link
Member

@jglick jglick commented Mar 26, 2018

Allows you to mark some APIs are being exposed for use outside the module only to consumers which opt in to this.

@reviewbybees

Copy link
Member

@daniel-beck daniel-beck left a comment

Choose a reason for hiding this comment

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

👍 This looks straightforward and pretty much how I imagined it would look like from previous discussion.

I wonder about the naming here though. Let me paint this bike shed a nicer shade of red:

  • I'm not sure beta conveys the reason for the existence of the annotation properly. It's not scary enough. AFAIUI, calling it experimental would be more suitable (and scary).
  • It's not necessarily used, just allowed.

Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

LGTM 🐝

* or when a special flag is set in the consuming module.
* This is the property {@code useBeta} with the value {@code true}.
*/
public class Beta extends DoNotUse {
Copy link
Member

Choose a reason for hiding this comment

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

Would be great to add support of some info/justification text to DoNotUse so that error/warning messages can provide more details. Not a part for this PR I'd guess

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 think you can just cover that in Javadoc.

Copy link
Member

@batmat batmat left a comment

Choose a reason for hiding this comment

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

LGTM

/**
* Loads a configuration setting from the environment, such as when configured by a Maven plugin.
*/
/*@CheckForNull*/ String getProperty(String key);
Copy link
Member

Choose a reason for hiding this comment

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

Why is @CheckForNull commented out?

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently this annotation library is not a dependency.

@jglick
Copy link
Member Author

jglick commented Mar 27, 2018

I wonder about the naming here though.

Anyone else feel one way or another? Easy enough to change now if so.

@jglick
Copy link
Member Author

jglick commented Mar 27, 2018

I'm not sure beta conveys the reason for the existence of the annotation properly.

Well, Guava calls it this. Indeed the API might be in widespread runtime use in production. The annotation is just saying that no claims are being made about its long-term compatibility to unknown callers, at least not yet. I do not think we need to make the phrasing “scary” if your build already breaks when you try to use the API by accident; callers have been fairly warned.

(BTW there is a FindBugs library for Guava’s @Beta. But I think we would rather use the access checker mojo here.)

It's not necessarily used, just allowed.

Of course; the way I would read

<properties>
  <useBeta>true</useBeta>
</properties>

in someone’s POM (via jenkinsci/plugin-pom#98) is ”this plugin is, or may be, using beta APIs”.

@batmat
Copy link
Member

batmat commented Mar 28, 2018

this plugin is, or may be, using beta APIs”.

Yeah, and in any case, if in doubt to quickly check I guess mvn -DuseBeta=false ... would make it fail if so.

@kohsuke kohsuke merged commit 1b7c553 into jenkinsci:master Apr 3, 2018
@kohsuke
Copy link
Member

kohsuke commented Apr 3, 2018

Released as 1.14

@jglick jglick deleted the beta branch April 3, 2018 15:24
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.

6 participants