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

[mail] Fix imap protocol setup error #7435

Closed
wants to merge 2 commits into from
Closed

Conversation

9037568
Copy link
Contributor

@9037568 9037568 commented Apr 21, 2020

While researching this problem in the community forum, I found this gremlin.

The protocol setup fails due to this line in the handler, which does nothing:

 protocol.concat("s");

So I fixed that and added some actual debug output.

Signed-off-by: 9037568 namraccr@gmail.com

Signed-off-by: 9037568 <namraccr@gmail.com>
@9037568 9037568 requested a review from J-N-K as a code owner April 21, 2020 01:51
@9037568 9037568 requested a review from clinique April 21, 2020 01:52
@TravisBuddy
Copy link

Travis tests have failed

Hey @9037568,
please read the following log in order to understand the failure reason. There might also be some helpful tips along the way.
It will be awesome if you fix what is wrong and commit the changes.

Signed-off-by: 9037568 <namraccr@gmail.com>
@TravisBuddy
Copy link

Travis tests were successful

Hey @9037568,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

@@ -23,4 +23,15 @@
@NonNullByDefault
public class POP3IMAPConfig extends BaseConfig {
public int refresh = 60;

public String debugPrint() {
StringBuffer buffer = new StringBuffer();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
StringBuffer buffer = new StringBuffer();
StringBuilder buffer = new StringBuilder();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Easy enough, but please explain why you think this is needed.

@@ -94,6 +94,7 @@ public void initialize() {
}
}

logger.debug("Configuration details:\r\n{}", config.debugPrint());
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
logger.debug("Configuration details:\r\n{}", config.debugPrint());
if(logger.isDebugEnabled()){
logger.debug("Configuration details:\r\n{}", config.debugPrint());
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This method is very low cost. Why do you feel the if conditional is needed here?

@@ -71,7 +71,7 @@ public void initialize() {
protocol = baseProtocol;

if (config.security == ServerSecurity.SSL) {
protocol.concat("s");
protocol = protocol.concat("s");
Copy link
Member

Choose a reason for hiding this comment

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

This is the only useful change here. Please remove all the log-bloating debugging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not going to happen. Debug logging is needed and useful.

Copy link
Member

Choose a reason for hiding this comment

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

As a Codeowner I prefer to keep the badly written style we had before.

@J-N-K J-N-K closed this Apr 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants