Skip to content

Updating org.netbeans.modules.gototest/ org.netbeans.modules.gsf.codecoverage and org.netbeans.modules.gsf.testrunner as public packages #7433

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

albilu
Copy link
Contributor

@albilu albilu commented Jun 4, 2024

Fix for #6871
Making gototest/gsf.codecoverage/gsf.testrunner public APIs

…coverage and org.netbeans.modules.gsf.testrunner as public packages
@mbien mbien added API Change [ci] enable extra API related tests tests labels Jun 4, 2024
@@ -69,6 +69,7 @@
</dependency>
</module-dependencies>
<public-packages>
<package>org.netbeans.modules.gsf.testrunner</package>
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO - it is not a good idea to make a public API from the code that was not originally designed to be an API.

@sdedic
Copy link
Member

sdedic commented Jun 6, 2024

Note that from a 'common' sense review (I've never worked with that API), the quality is ... strange. Take for example this: https://github.com/apache/netbeans/blob/master/ide/gsf.testrunner/src/org/netbeans/modules/gsf/testrunner/plugin/CommonTestUtilProvider.java#L31
which is not documented at all. The documentation is perhaps here:
https://github.com/apache/netbeans/blob/master/java/java.testrunner/src/org/netbeans/modules/java/testrunner/CommonTestUtil.java#L286

  • there's a fixed set of keys. The set is NOT extensible from other modules.
  • each key has its own data type, which is specified by the documentation

... that would normally lead to a final settings bean class or r/o class with a builder rather than untyped map.

Nobody cared that much since it is a friend API with a limited impact, but if we ever make it public, it has to be maintained forever. Going public is the last chance the API can be cleaned up and made future-changes-compatible. It's not that just a "flag is flipped".

@lahodaj
Copy link
Contributor

lahodaj commented Jun 6, 2024

I concur with @dbalek and @sdedic - while exposing a package to be public is easy, we then need to take care of it as an API for a long time/forever, which is difficult code that was originally arbitrary implementation code.

I would suggest to first find out and describe what are the usecases/what you need, and then we can talk about how to extend the API to support that usecases. And do the API extension in a way that can be supported in long term.

Thanks.

Copy link
Member

@neilcsmith-net neilcsmith-net left a comment

Choose a reason for hiding this comment

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

I don't understand this PR. I 100% support your original request to remove the friend restrictions on the API for these modules. I've consistently said we should not use the friend feature for this purpose. Unstable / in-development APIs should be marked as such, and opt-in, without expecting them never to break. The JDK has at least woken up to the importance of incubating and preview features, and we should too!

However, this PR doesn't make that change?! What are you trying to achieve and why?

@mbien mbien marked this pull request as draft June 26, 2024 05:13
@albilu
Copy link
Contributor Author

albilu commented Sep 9, 2024

Thanks all: @neilcsmith-net @sdedic @dbalek for your feedbacks and sorry for the late response.

However, this PR doesn't make that change?! What are you trying to achieve and why?

I am trying to make these API public so that plugin developpers are no longer restricted by the friend restriction: meaning avoid to have to provide a new build of a plugin depending on these API for each Netbeans release.

From this DEV thread https://lists.apache.org/thread/rdwh4vh07zxgbxyjfzfhxd8yjk7o23wv, Laszlo Kishalmi seemed ok with these API going public. That's why i made the PR.

I would suggest to first find out and describe what are the usecases/what you need, and then we can talk about how to extend the API to support that usecases. And do the API extension in a way that can be supported in long term.

Here are what i need:

  • Avoid to have to provide a new build of a plugin (in this case https://github.com/albilu/netbeansPython) for each Netbeans release. It is very annoying.
  • More generally the usecase is to ease Language support plugin development for all Plugin developpers.

@lahodaj : I understand the concerns from everyone. I will be glad for any other solution to achieve this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Change [ci] enable extra API related tests tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants