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

Alternative Standalone Sampler #4052

Merged
merged 4 commits into from
Apr 29, 2022

Conversation

jtulach
Copy link
Contributor

@jtulach jtulach commented Apr 29, 2022

Continuous work on enso-org/enso#3389 has revealed that it is not possible to exclude five NetBeans libraries when using sampler module standalone. To address that this PR continues in #4047:

  • d66ce57 duplicates the tests to check not only the InternalSampler, but also existing CLISampler
  • 9f4ec39 adds yet another StandaloneSampler and its test

The test creates own ClassLoader and loads just the sampler JAR, without dependencies on other NetBeans modules (so Lookup isn't available) and then performs the AbstractSamplerBase tests. This guarantees that all three samplers behaves the same.

It'd be ideal if these changes could be uploaded to Maven central soon, as enso-org/enso#3389 can only consume released binaries.

@jtulach jtulach added this to the NB14 milestone Apr 29, 2022
@jtulach jtulach requested review from ebarboni and sdedic April 29, 2022 09:49
@jtulach jtulach self-assigned this Apr 29, 2022
@neilcsmith-net
Copy link
Member

Curious why this is going into delivery and not master?

@ebarboni
Copy link
Contributor

I accepted #4047 no more thinking on just accept bug fixes.
I merge this one as works for me + green and merge rc2

@ebarboni ebarboni merged commit d5f29c1 into apache:delivery Apr 29, 2022
@JaroslavTulach
Copy link

Curious why this is going into delivery and not master?

I need Maven bits, so I'd like to get this into NetBeans 14.

@matthiasblaesing
Copy link
Contributor

Curious why this is going into delivery and not master?

I need Maven bits, so I'd like to get this into NetBeans 14.

I think the point that @neilcsmith-net was trying to make is, that this does not look like a PR helping to stabilize NB14. The description of the issues shows IMHO that these are feature changes, so that another dependency gets a lower number of dependencies.

@neilcsmith-net
Copy link
Member

Yes, exactly what @matthiasblaesing said! This doesn't seem to fit the normal criteria for merges to delivery. Not that there's a problem with bending the rules occasionally, just like to see the reasoning (cost/benefit) in the PR for doing so.

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.

5 participants