Skip to content

Conversation

@acoburn
Copy link
Member

@acoburn acoburn commented Apr 6, 2018

Resolves #92

Once we have agreement on this approach, I can apply it to #88, allowing the test suite to adjust to varying levels of LDP container support.

I decided not to include a default implementation in the interface -- if we want implementations to be clear about what is supported, then I thought it makes more sense to simply require an implementing class to implement the method.

I am also making use of a new named individual in the Trellis ontology trellis:InvalidInteractionModel, which is used with the ldp:constrainedBy link header in the case of an error condition.

@acoburn acoburn requested a review from ajs6f April 6, 2018 18:41
Copy link
Member

@ajs6f ajs6f left a comment

Choose a reason for hiding this comment

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

Beautiful, one little style thing (in three or four places) but this is the right idea.

allModes.add(ACL.Read);
allModes.add(ACL.Write);

allInteractionModels.add(LDP.Resource);
Copy link
Member

Choose a reason for hiding this comment

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

If we have Guava on line (I think we do) maybe use something from Sets here to construct and fill the set?

Copy link
Member Author

Choose a reason for hiding this comment

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

Guava isn't a compile dependency, but I have no problem with adding it as a test dependency (if it's not already there).

allInteractionModels.add(LDP.Container);
allInteractionModels.add(LDP.BasicContainer);
allInteractionModels.add(LDP.DirectContainer);
allInteractionModels.add(LDP.IndirectContainer);
Copy link
Member

Choose a reason for hiding this comment

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

See above about Sets.

allInteractionModels.add(LDP.Container);
allInteractionModels.add(LDP.BasicContainer);
allInteractionModels.add(LDP.DirectContainer);
allInteractionModels.add(LDP.IndirectContainer);
Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

@acoburn
Copy link
Member Author

acoburn commented Apr 6, 2018

I think the Trellis ontology should change trellis:InvalidInteractionModel to trellis:UnsupportedInteractionModel, which more accurately captures what is going on. I can update that as part of this PR or separately. WDYT?

@ajs6f
Copy link
Member

ajs6f commented Apr 6, 2018

Just waiting for Travis et al to complete

@ajs6f
Copy link
Member

ajs6f commented Apr 6, 2018

Good?

@acoburn
Copy link
Member Author

acoburn commented Apr 6, 2018

@ajs6f yes, feel free to squash/merge etc whenever you'd like

@ajs6f ajs6f merged commit 4b6ab49 into master Apr 6, 2018
@ajs6f ajs6f deleted the trellis-92 branch April 6, 2018 20:25
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