WINDUP-2018 Enable tattletale by default #1330
Conversation
mrizzi
force-pushed
the
WINDUP-2018
branch
2 times, most recently
from
151133f
to
1c9a473
Compare
|
Commit e44fc8a has the changes needed for implementing the requirements from this comment on JIRA on. |
| Boolean disableReport = (Boolean) event.getGraphContext().getOptionMap().get(DisableTattletaleReportOption.NAME); | ||
| Collection<String> targets = (Collection<String>) event.getGraphContext().getOptionMap().get(TargetOption.NAME); | ||
| boolean eapTarget = targets.stream().anyMatch(target -> target.startsWith("eap")); | ||
| if (eapTarget && disableReport != null && disableReport && (enableReport == null || !enableReport)) |
There was a problem hiding this comment.
for a tiny bit more readability I would use Boolean.valueOf(disableReport) here and for the enableReport logic as well
mrizzi
force-pushed
the
WINDUP-2018
branch
2 times, most recently
from
f649a37
to
8fb1e9b
Compare
...etale/addon/src/main/java/org/jboss/windup/rules/apps/tattletale/TattletaleRuleProvider.java
Outdated
Show resolved
Hide resolved
|
I have to say that from the developer point of view is a bit criptic having enableOption and disableOption at the same time, and classes with a name that includes the enabling status. |
...ddon/src/main/java/org/jboss/windup/rules/apps/tattletale/DisableTattletaleReportOption.java
Show resolved
Hide resolved
…ortGenerationEnabled
| package org.jboss.windup.tests.bootstrap.migrate; | ||
|
|
||
| import org.jboss.windup.bootstrap.Bootstrap; | ||
| import org.jboss.windup.rules.apps.tattletale.TattletaleRuleProvider; |
There was a problem hiding this comment.
it seems this import is not used
There was a problem hiding this comment.
good catch, thanks.
Also TattletaleEnablementByDefaultTest.java seems to have some
...s/src/test/java/org/jboss/windup/tests/bootstrap/migrate/DisableTattletaleParameterTest.java
Show resolved
Hide resolved
|
|
||
| private boolean isTattleTaleReportGenerationEnabled(GraphRewrite event) | ||
| { | ||
| Boolean enableReport = (Boolean) event.getGraphContext().getOptionMap().get(EnableTattletaleReportOption.NAME); |
There was a problem hiding this comment.
We could convert this to :
boolean enableReport = (boolean) event.getGraphContext().getOptionMap().getOrDefault(EnableTattletaleReportOption.NAME, Boolean.FALSE);
And this way we can avoid using BooleanUtils and make it clearer.... what do you think ?
There was a problem hiding this comment.
I'm afraid this would lead to a NPE in case there's no such a key due to the unboxing done on null
There was a problem hiding this comment.
Ah sorry, you proposed the default version even if i don't think we should have the options default here.
Would you prefer having the boolean instead of BooleanUtils for readability?
Or, talking about code, if we prefer
Boolean disableReport = (Boolean) event.getGraphContext().getOptionMap().get(DisableTattletaleReportOption.NAME);
if (BooleanUtils.isTrue(disableReport))or
boolean disableReport = (boolean) event.getGraphContext().getOptionMap().getOrDefault(DisableTattletaleReportOption.NAME, Boolean.FALSE);
if (disableReport)There was a problem hiding this comment.
well, a pure boolean is easier to read, and in fact if we dont have the value in the map we consider them as false, is what BooleanUtils is doing
public static boolean isTrue(Boolean bool) { if (bool == null) { return false; } else { return bool; } }
...etale/addon/src/main/java/org/jboss/windup/rules/apps/tattletale/TattletaleRuleProvider.java
Outdated
Show resolved
Hide resolved
--enableTattletaleoption with the following console warning:--disableTattletaleoption--enableTattletaleand--disableTattletale) are provided. If that's the case the executions stops with the following message:--enableTattletale, only--disableTattletale, both options)