Skip to content

Conversation

@ajs6f
Copy link
Member

@ajs6f ajs6f commented Mar 20, 2018

This commit introduces protected methods for most of the parts of a running Trellis instance, in order to make subclassing TrellisApplication less filled with cut-and-paste.

@ajs6f
Copy link
Member Author

ajs6f commented Mar 20, 2018

This PR will fail to build because checkstyle does not like the import order. I tried every permutation I could think of and couldn't get it to work.

Does anyone who uses Eclipse have a formatter configuration that agrees with whatever Checkstyle spec is in use?

@christopher-johnson
Copy link
Member

the imports have to be alphabetical. So import javax.jms.JMSException would be after import io.dropwizard.Application

import static org.trellisldp.app.TrellisUtils.getRDFConnection;
import static org.trellisldp.app.TrellisUtils.getWebacConfiguration;

import javax.jms.JMSException;
Copy link
Member

Choose a reason for hiding this comment

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

This should follow the import io.... section

@ajs6f
Copy link
Member Author

ajs6f commented Mar 20, 2018

Thanks, guys.

@acoburn
Copy link
Member

acoburn commented Mar 20, 2018

The only issue here is that the TriplestoreResourceService implements both the AuditService and ResourceService interfaces, and a different persistence impl may separate those things. Instead, I wonder if there shouldn't rather be some sort of shared init() (or PostConstruct-annotated) method and the protected methods are more like: get*Service() instead of build*Service. That way, if some of those objects implement multiple interfaces, there wouldn't be an issue.

@ajs6f
Copy link
Member Author

ajs6f commented Mar 20, 2018

Oh, shoot, you're right, and what's more, I think I did that. Let me do a little quick juggling and see if I can keep this PR simple, and yes, I'll try to use some fields and an init for this. That sounds good. The next commit might be a little while coming because the storm is getting worse here and I might want to get myself home before digging into this.

@ajs6f
Copy link
Member Author

ajs6f commented Mar 20, 2018

Looks like the JDK10 Travis build failed for unrelated reasons (couldn't download the JDK).

@acoburn
Copy link
Member

acoburn commented Mar 20, 2018

Don't worry about the JDK 10 build -- I figured that the download URL would change today. I'll fix that separately.

@acoburn
Copy link
Member

acoburn commented Mar 20, 2018

The JDK 10 download issue has been resolved: 4ceeb23

@ajs6f
Copy link
Member Author

ajs6f commented Mar 20, 2018

Better?


private MementoService mementoService;

private TriplestoreResourceService resourceService;
Copy link
Member

Choose a reason for hiding this comment

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

Presumably, a subclass would not use this field? In other words, doesn't this add a hard dependency on the Triplestore implementation?

@ajs6f
Copy link
Member Author

ajs6f commented Mar 20, 2018

Arg! Fixed. My excuse is that I just got home from the snow.

Copy link
Member

@acoburn acoburn left a comment

Choose a reason for hiding this comment

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

I think the return types should be made more generic.

return new TrellisCache<>(cache);
}

protected JenaIOService buildIoService(final NamespaceService namespaceService,
Copy link
Member

Choose a reason for hiding this comment

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

JenaIOService => IOService.

return new JenaIOService(namespaceService, profileCache, TrellisUtils.getAssetConfiguration(config));
}

protected FileBinaryService buildBinaryService(final IdentifierService idService) {
Copy link
Member

Choose a reason for hiding this comment

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

FileBinaryService => BinaryService

config.getBinaryHierarchyLength());
}

protected NamespacesJsonContext buildNamespaceService() {
Copy link
Member

Choose a reason for hiding this comment

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

NamespacesJsonContext => NamespaceService

return new NamespacesJsonContext(config.getNamespaces());
}

protected FileMementoService buildMementoService() {
Copy link
Member

Choose a reason for hiding this comment

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

FileMementoService => MementoService

return new FileMementoService(config.getMementos());
}

protected UUIDGenerator buildIdService() {
Copy link
Member

Choose a reason for hiding this comment

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

UUIDGenerator => IdentifierService

@ajs6f
Copy link
Member Author

ajs6f commented Mar 20, 2018

Arg. Sorry, doing too much at once.

@acoburn acoburn merged commit 4685e12 into trellis-ldp:master Mar 20, 2018
acoburn pushed a commit that referenced this pull request Mar 20, 2018
Factoring apart component builds for easier overriding
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.

3 participants