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

Remove misleading @Restricted annotation on package #235

Closed
wants to merge 1 commit into from

Conversation

darxriggs
Copy link
Contributor

A package can be annotated. BUT: No restriction is applied!

@jetersen
Copy link
Member

jetersen commented Oct 5, 2019

I'd prefer if the API was moved to a separate plugin or we found a way to combine efforts working on an existing API library

@darxriggs
Copy link
Contributor Author

I also opened this issue jenkinsci/lib-access-modifier#19.

@oleg-nenashev
Copy link
Member

+1 for a separate plugin. No opinion about this PR, I am not sure maintainers want to open API

@stale
Copy link

stale bot commented Dec 10, 2019

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Dec 10, 2019
@oleg-nenashev
Copy link
Member

Ping @Casz for the final decision

@stale stale bot removed the stale label Dec 10, 2019
@oleg-nenashev oleg-nenashev requested review from jetersen and acdesouza and removed request for acdesouza December 10, 2019 13:43
@jetersen
Copy link
Member

🤷‍♂️

@darxriggs
Copy link
Contributor Author

darxriggs commented Dec 10, 2019

Maybe I missed to add enough information. Seeing your comments I am not sure if the goal of this pull request was fully understood. At least I did not completely understand your comments. So let me try again.

  • The package-info.java file was added in 41b4d13 only to provide the @Restricted(NoExternalUse.class) annotation on the com.cloudbees.jenkins.plugins.bitbucket.api package.
  • Doing so has no effect at all or none that I am aware of.
  • It's still possible to use this plugin as a dependency of another plugin and to create an instance of a class inside the mentioned package - for example new BitbucketException("message").
  • Therefore I removed package-info.java to get rid of the confusion.

In addition I am asking myself why was the api be restricted in the first place. That's the whole idea of an api to be the interface that can be used. Maybe I am missing something.

@jetersen
Copy link
Member

As I recall this was added to avoid users depending on this plugin to only use the API.
Would be nice to create a API plugin much like
https://github.com/jenkinsci/apache-httpcomponents-client-4-api-plugin
https://github.com/jenkinsci/gitlab-api-plugin

Copy link
Member

@jetersen jetersen left a comment

Choose a reason for hiding this comment

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

I am fine with this either way

@darxriggs
Copy link
Contributor Author

Who is merging it?

@amuniz
Copy link
Member

amuniz commented Dec 16, 2019

Doing so has no effect at all or none that I am aware of.

I wasn't aware of this issue in the access-modifier-checker. It should be fixed there instead of removing the restriction here.

That's the whole idea of an api to be the interface that can be used

This package is named api because it contains the harness to access Bitbucket's API, not because it is a public API itself.

I wouldn't open this for usages by other plugins, cause doing so would make the evolution of this internal part of the code much more difficult (would need to keep backward compatibility).

The right thing is to create a bitbucket-api plugin as others have already pointed out in this PR.

That's my opinion, but I let @Casz to decide as the main maintainer.

Remove misleading @restricted annotation from the package.
A package can be annotated but no restriction is applied!

Therefore annotating all classes in the package to take effect.
@darxriggs darxriggs force-pushed the misleading-restriction branch from fdae124 to ea5c7c5 Compare December 18, 2019 22:05
@darxriggs
Copy link
Contributor Author

Ok now I understood what this api is about.

I added a small Javadoc to the package and annotated all classes in the package to properly restrict them from external use as originally intended.

@stale
Copy link

stale bot commented Feb 16, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Feb 16, 2020
@stale stale bot closed this Feb 23, 2020
@jetersen jetersen reopened this Feb 24, 2020
@stale stale bot removed the stale label Feb 24, 2020
@stale
Copy link

stale bot commented Apr 25, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Apr 25, 2020
@stale stale bot closed this May 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants