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

Logging factory #267

Merged
merged 9 commits into from
Sep 2, 2016
Merged

Logging factory #267

merged 9 commits into from
Sep 2, 2016

Conversation

solind
Copy link

@solind solind commented Aug 24, 2016

Here is a separate PR for the LoggingFactory injection we discussed in issue #264.

@solind
Copy link
Author

solind commented Aug 24, 2016

I don't understand why the license check failed, I copied the Copyright from another source file in the project.

@solind
Copy link
Author

solind commented Aug 24, 2016

Regarding the remaining nits from codacy, as you can see this is a pretty large bunch of changes. It's complaining about member variable declarations that are essentially unmoved from the pre-refactored code base, and I had no intention of re-factoring the classes to meet the new style guidelines. I hope that doesn't pose a significant issue.

private final Buffer.PlainBuffer buffer;

private byte[] EXPECTED_START_BYTES = new byte[] {'S', 'S', 'H', '-'};

public IdentificationStringParser(Buffer.PlainBuffer buffer) {
public IdentificationStringParser(LoggerFactory loggerFactory, Buffer.PlainBuffer buffer) {
this.log = loggerFactory.getLogger(IdentificationStringParser.class);
Copy link
Owner

Choose a reason for hiding this comment

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

indentation is wrong

Copy link
Author

Choose a reason for hiding this comment

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

I fixed the indentation (sorry I use tabs for 8 spaces reflexively). What’s up with the “license” issue?

Copy link
Owner

Choose a reason for hiding this comment

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

If you add new classes, easiest is to run gradle licenseFormat. This will fix the license headers.

@@ -174,15 +174,15 @@ protected boolean isMyType(Key key) {
},

ED25519("ssh-ed25519") {
private final Logger logger = LoggerFactory.getLogger(KeyType.class);
private final Logger log = LoggerFactory.getLogger(KeyType.class);
Copy link
Owner

Choose a reason for hiding this comment

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

Isn't this using the wrong LoggerFactory now?

Copy link
Author

Choose a reason for hiding this comment

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

There are a few places — like these static initializers — where it’s not possible to inject a LoggerFactory. I could use the new LoggerFactory.DEFAULT instead, but instead I just left them as-is, as the result is identical.

On Aug 24, 2016, at 6:39 AM, Jeroen van Erp notifications@github.com wrote:

In src/main/java/net/schmizz/sshj/common/KeyType.java #267 (comment):

@@ -174,15 +174,15 @@ protected boolean isMyType(Key key) {
},

 ED25519("ssh-ed25519") {
  •    private final Logger logger = LoggerFactory.getLogger(KeyType.class);
    
  •    private final Logger log = LoggerFactory.getLogger(KeyType.class);
    
    Isn't this using the wrong LoggerFactory now?


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub https://github.com/hierynomus/sshj/pull/267/files/f63a88ec9f1ca21db2d18eb68056fcf51a107ff5#r76040618, or mute the thread https://github.com/notifications/unsubscribe-auth/ABIPfWE18KqAoYxuTzYWKzAfdlJvq0IWks5qjC1WgaJpZM4JrhJ-.

@solind
Copy link
Author

solind commented Aug 24, 2016

The PacketReaderTest fails with a NullPointerException because a mock STFPEngine's getLoggerFactory method returns null.

It seems silly to do a null check for a final variable... it also seems silly to vastly complicate the packet reader test by creating a real SFTPEngine.

I'll see if I can use Mockito to return a LoggerFactory.DEFAULT.

@solind
Copy link
Author

solind commented Aug 26, 2016

Hi Jeroen, any thoughts on this PR? I have a few additional modifications that are waiting in the wings (not logging-related).

@hierynomus
Copy link
Owner

I'll have a look after the weekend. I've quickly looked through it, but want a more in-depth look before I merge it.

@solind
Copy link
Author

solind commented Aug 30, 2016

Hey Jeroen, any code review feedback yet? The feature is working well in my integration with our product, I'm eager to move forward.

@hierynomus hierynomus merged commit 1dad19c into hierynomus:master Sep 2, 2016
@joval joval deleted the logging-factory branch September 2, 2016 11:45
@hierynomus hierynomus mentioned this pull request Sep 2, 2016
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.

2 participants