-
Notifications
You must be signed in to change notification settings - Fork 3
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
#59 Added the ability to suppress individual manual action notifications #60
#59 Added the ability to suppress individual manual action notifications #60
Conversation
README.md
Outdated
</configuration> | ||
``` | ||
To get the key values of the messages produced by a build, run the fermenter | ||
plugin with the goal of "find-messages", like this: `mvn org.technologybrewery.fermenter:fermenter-mda:find-messages` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A: I don't like the casual tone of "like this"
import java.util.List; | ||
import java.util.Map; | ||
import java.util.Set; | ||
import java.util.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A: Would prefer a less exhaustive import
5961528
to
a4c06fd
Compare
@@ -101,6 +105,14 @@ static void addNotificationToPassedMap(String targetFile, Notification notificat | |||
* NOTIFICATION_DIRECTORY_PATH. | |||
*/ | |||
public void recordNotifications(MavenProject project) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q/S: Where is this being used? I wouldn't think we'd ever want to call this method without supplying the suppression configuration.
@@ -190,6 +233,7 @@ private void emitNotifications() throws IOException { | |||
} | |||
|
|||
logger.warn("To disable these messages, please use -Dfermenter.hide.manual.actions=true"); | |||
logger.warn("To disable specific messages, please add their message keys to the suppressedMessages list. See the fermenter docs for more info."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A: Fermenter should be capitalized in this case, and a link wouldn't hurt
logger.warn("Message Keys:"); | ||
logger.warn("------------------------------------------------------------------------"); | ||
for(String key: keys) { | ||
logger.warn(key); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
S: How would I know what key goes with what message? Should this not just be an extension of the usual notification printing such that each notification is preceded by its corresponding key? I think that also streamlines the code a bit. We could potentially just add a boolean to getNotificationAsString
for shouldIncludeKey
. Then no matter where/how you're displaying the notification, it will/will not include the key as appropriate.
* Utility tool for capturing the list of key values for the messages that a project will display during a build. | ||
*/ | ||
@Mojo(name = "find-messages", defaultPhase = LifecyclePhase.GENERATE_SOURCES) | ||
public class FindMessages extends AbstractMojo { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
S: We should document this and not leave it as a footnote to the documentation of suppressedMessages
. IMO, this Mojo isn't any more useful than the configuration of fermenter.print.message.keys
, so I'd be ok with just removing this and updating the docs to indicate leveraging this flag.
if (notification instanceof VelocityNotification) { | ||
VelocityNotification velocityNotification = (VelocityNotification) notification; | ||
velocityNotification.writeExternalVelocityContextProperties(project.getBasedir()); | ||
if(suppressedMessages == null || (suppressedMessages != null && !suppressedMessages.contains(subMapEntry.getValue().getKey()))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A:
if(suppressedMessages == null || (suppressedMessages != null && !suppressedMessages.contains(subMapEntry.getValue().getKey()))) { | |
if(suppressedMessages == null || !suppressedMessages.contains(subMapEntry.getValue().getKey())) { |
I'd also pull the assignment Notification notification = subMapEntry.getValue()
up above the if statement to make the check more readable.
public void a_notification_key_to_suppress() throws Throwable { | ||
MavenSession session = testCase.newMavenSession(); | ||
Notification notification = new VelocityNotification("test-message-id", new HashSet<>(), "templates/notifications/sample.notification.vm"); | ||
NotificationCollector.addNotification("test-message-id", notification); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q/A: Why not have the generator add the notification so it mirrors the expected usage?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To fix the build issue, we need to apply similar changes to the build workflow as in TechnologyBrewery/habushu#125
32e5ae1
to
610c43c
Compare
…action notifications
ab88426
to
7a89cca
Compare
No description provided.