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

Enforce or drop HasInternalProtocol annotation #8236

Closed
lptr opened this issue Jan 15, 2019 · 7 comments
Closed

Enforce or drop HasInternalProtocol annotation #8236

lptr opened this issue Jan 15, 2019 · 7 comments
Labels

Comments

@lptr
Copy link
Member

lptr commented Jan 15, 2019

We used to use @HasInternalProtocol to signal that a public type has an accompanying internal type to go with it. The annotation is used inconsistently, sometimes not appearing on types that do have an accompanying type, while sometimes appearing on types that don't (or not anymore). The practical value of having this annotation present in a consistent way is not obvious either.

One option is to decide that this annotation is useful, and then enforce its use. One way to do that is by annotating the accompanying internal type with a reference to its public type (@AccompanyingInternalTypeOf(PublicType.class)), and make sure that for every @HasInternalProtocol-annotated type we have at least one such reference at all times, and vice versa.

The other option is to remove @HasInternalProtocol. If we do so, we should consider capturing a different quality of public types, which is their extensibility by user code. We should probably annotate all public types (there should be few) that are supposed to be extended/implemented by user code with something like @Extensible.

@adammurdoch
Copy link
Member

I would remove it, and replace it with something like an @Extensible annotation.

@ldaley
Copy link
Member

ldaley commented Jan 16, 2019

I suggest also having an inverse annotation and verifying that all applicable types are annotated.

The reason being is that the primary purpose is/was to communicate to users. Relying on the omission of annotations to communicate “you should not extend/implement this” seems risky to me. I don't think enough users would internalise “only extend @Extensible” for it to be effective. Sure, some people will miss @NonExtensible, but it should reduce the number.

@ldaley
Copy link
Member

ldaley commented Jan 16, 2019

The reason being is that the primary purpose is/was to communicate to users.

I am aware it did not do this well.

@lptr
Copy link
Member Author

lptr commented Jan 16, 2019

We could have static validation to see if a plugin extended something not-extendable, regardless of how it is marked (and perhqps require an explicit opt-in to allow it).

This is a somewhat different topic, but we should find a way to enforce validations, as currently we don’t have a good way to say that “if you are developing code for Gradle, you should adhere to these rules”. One way could be to allow adding the gradleApi() dependency only by applying a plugin. This plugin could enable all the validations we want.

@ldaley
Copy link
Member

ldaley commented Jan 17, 2019

I think we should prioritise consistent and correct usage on our side before the user side.

@stale
Copy link

stale bot commented Aug 5, 2020

This issue has been automatically marked as stale because it has not had recent activity. Given the limited bandwidth of the team, it will be automatically closed if no further activity occurs. If you're interested in how we try to keep the backlog in a healthy state, please read our blog post on how we refine our backlog. If you feel this is something you could contribute, please have a look at our Contributor Guide. Thank you for your contribution.

@stale stale bot added the stale label Aug 5, 2020
@stale
Copy link

stale bot commented Aug 26, 2020

This issue has been automatically closed due to inactivity. If you can reproduce this on a recent version of Gradle or if you have a good use case for this feature, please feel free to reopen the issue with steps to reproduce, a quick explanation of your use case or a high-quality pull request.

@stale stale bot closed this as completed Aug 26, 2020
@wolfs wolfs closed this as not planned Won't fix, can't repro, duplicate, stale Sep 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants