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

Adds SingleThreadedLogger to be able to keep a footer for where progress indication will go. #1326

Merged
merged 12 commits into from
Dec 12, 2018

Conversation

coollog
Copy link
Contributor

@coollog coollog commented Dec 7, 2018

Part of #1297

This will be gradually improved once it is used in the LogEventHandler.

@coollog coollog requested a review from a team December 7, 2018 21:36
Copy link
Member

@chanseokoh chanseokoh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got some questions. Maybe due to not being able to see the big picture.

* console
* @return a {@link Future} to track completion
*/
public Future<Void> log(Runnable messageLogger) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying to understand why this needs to be a Runnable instead of a String. Isn't it that in the end what this Runnable will do is to write some string to plainLogger? Actually, what does "plain" mean here? Also, is it that this "message logger" (who will log a message to the console according to the Javadoc) is a different logger than the plain logger? (I didn't get what these two loggers will be in the end.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is for the user to pass like (Maven) () -> log.info("log message") or other logging method, so that this class is independent of the actual different logging levels.

plain is for a logging method similar to just plain System.out.print, so it will be like lifecycle for Gradle and probably System.out.print for Maven (might need to do some things differently if wanting to use Maven info logging).

Copy link
Member

@chanseokoh chanseokoh Dec 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, now I see the intention. Basically, this class is the LogEvent handler (or will be used by whatever LogEvent handler) to "move" the footer (the multi-line progress display) down by one line after logging the message (if it gets printed to the screen, for example). This is done by calling this log() message. And at the same time, this class is the ProgressEvent handler (or will be used by whatever ProgressEvent handler) to update the progress display, which is done by setFooter(). I see where my confusion came from: first, the potential mix of LogEvent and ProgressEvent; secondly, the mix of two output channels, one for messageLogger and plainLogger (for example, one going out to a file and the other going out to a screen); and the messageLogger is basically an unrestricted lambda. Not that I am suggesting something concrete here... don't really have an idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, this will be used by the LogEventHandler. A ProgressLogger will be the one that handles ProgressEvents and directly calls setFooter on this or set the footer via a LogEvent type of CHANGE_FOOTER or something.

@coollog coollog requested a review from a team December 10, 2018 16:58
@coollog coollog requested a review from a team December 10, 2018 19:30
@coollog coollog requested a review from a team December 10, 2018 23:23
Copy link
Member

@chanseokoh chanseokoh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK from my side. Renaming the class sounds good.

@coollog coollog requested a review from a team December 11, 2018 16:09
Copy link
Member

@chanseokoh chanseokoh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I have one comment after you introduced shutdown.

@coollog coollog merged commit 483c84a into master Dec 12, 2018
@coollog coollog deleted the progress-8-LogEventHandler branch December 12, 2018 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants