-
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
#16: 🐛 fix forked session issues with manual action notifications… #17
Conversation
...-mda/src/main/java/org/technologybrewery/fermenter/mda/notification/NotificationService.java
Outdated
Show resolved
Hide resolved
...-mda/src/main/java/org/technologybrewery/fermenter/mda/notification/NotificationService.java
Outdated
Show resolved
Hide resolved
...-mda/src/main/java/org/technologybrewery/fermenter/mda/notification/NotificationService.java
Outdated
Show resolved
Hide resolved
File outputFile = new File(projectParentFile, fileName + "-" + i++ + ".txt"); | ||
try { | ||
FileUtils.forceMkdir(projectParentFile); | ||
FileUtils.write(outputFile, subMapEntry.getValue().getNotificationAsString(), Charset.defaultCharset()); |
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: Minor, but java.nio.Files can do both of these things without a 3rd party lib.
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.
Fair, though this library is already there, so feels like 6 and one half
} | ||
} | ||
|
||
if (manualActionCount > 0) { |
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: We are baking in the notion that "notifications" == "manual actions". Maybe/probably fine for incremental addition, but I could see this being more universal, e.g. profile deprecation notifications.
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.
Yeah - agree. Debated this but didn't have a good example of what wouldn't be a manual action that we are outputting. Easy to refactor to a more general concept if we want to, however.
...-mda/src/main/java/org/technologybrewery/fermenter/mda/notification/NotificationService.java
Outdated
Show resolved
Hide resolved
cd08667
to
5f9c61b
Compare
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.
Looks good.
…ite notifications and then read them rather than passing around in the Maven Session; with PR feedback
5f9c61b
to
8d8e980
Compare
…; write notifications and then read them rather than passing around in the Maven Session