WELD-2374 Add option to disable Jandex in SE and Servlet environment. #1657
Conversation
|
I think we could share Also I would add a boolean |
manovotn
force-pushed
the
weld2374
branch
3 times, most recently
from
8ed08d8
to
7b7a261
Compare
| @Test | ||
| public void testDeploymentWorksBecauseJandexIsNotUsed() { | ||
| try (WeldContainer container = new Weld().initialize()) { | ||
| // no-op, if the option does not work, this will crash |
There was a problem hiding this comment.
What about to add some basic assertion like container is running or something like that.
There was a problem hiding this comment.
Can do that, although it makes no real difference. If the option does not work, you get an exception during bootstrap.
|
BTW apart from the SE test I did test it manually with numberguess on Tomcat 8.5.5. |
| } catch (Exception e) { | ||
| throw CommonLogger.LOG.unableToInstantiate(Jandex.JANDEX_DISCOVERY_STRATEGY_CLASS_NAME, | ||
| Set<Class<? extends Annotation>> initialBeanDefiningAnnotations, boolean jandexStrategyDisabled) { | ||
| if (jandexStrategyDisabled) { |
There was a problem hiding this comment.
I think we could move this check inside the if (Jandex.isJandexAvailable(resourceLoader)) block. It makes more sense to only show the log message if jandex is on the classpath. WDYT?
There was a problem hiding this comment.
Hmm, we could but I don't like it.
I specifically made it this way, so that we don't even need to look whether there is any Jandex on CP if this option is set.
| } catch (Exception e) { | ||
| throw CommonLogger.LOG.unableToInstantiate(Jandex.JANDEX_DISCOVERY_STRATEGY_CLASS_NAME, | ||
| if (jandexStrategyDisabled) { | ||
| CommonLogger.LOG.jandexDiscoveryStrategyDisabled(); |
There was a problem hiding this comment.
@mkouba changed based on your suggestion
|
I think we're ready to merge now. @manovotn could you send a PR for 2.4 branch? |
|
Done in #1659 |
No description provided.