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

Translate CND to RDFS, and expose it over the REST API #101

Merged
merged 2 commits into from Oct 31, 2013
Merged

Conversation

cbeer
Copy link
Contributor

@cbeer cbeer commented Jul 19, 2013

@ajs6f
Copy link
Contributor

ajs6f commented Jul 19, 2013

Looks good-- do you want to hybridize with the ontology I did earlier?

@awoods
Copy link

awoods commented Jul 31, 2013

This is a great addition. What are your concerns? and what do you think needs to be added to make it ready for primetime?

@cbeer
Copy link
Contributor Author

cbeer commented Jul 31, 2013

Blocked by MODE-1997. Expecting MODE 3.4 to land soon.

@ajs6f
Copy link
Contributor

ajs6f commented Jul 31, 2013

+1 to @awoods' praise. This is exactly the direction we should be going.

@cbeer
Copy link
Contributor Author

cbeer commented Oct 28, 2013

@awoods I'd prefer if we kept these as two separate commits. The type-safe iterator has utility beyond the JCR => RDFS translation.

@cbeer
Copy link
Contributor Author

cbeer commented Oct 28, 2013

@ajs6f can you review the RdfStream work? Is it in line with your thinking?

(also, I left the Namespaced => Resource logic in my class for now. We can break it out after you land your refactor, i think)

@awoods
Copy link

awoods commented Oct 28, 2013

@cbeer, sounds good. Are you associating this branch with both of the following tickets:
https://www.pivotaltracker.com/story/show/53722449
https://www.pivotaltracker.com/story/show/53804961

@cbeer
Copy link
Contributor Author

cbeer commented Oct 28, 2013

@awoods nope. #53804961 has been complete and in master for some time.

@ajs6f
Copy link
Contributor

ajs6f commented Oct 28, 2013

Looks okay. I would have done a few things differently:

Instead of building up Collections, I would have introduced Functions and used transform() and our ZippingIterator. That would make things lazier.

I would be using the @mock annotation on fields a bit in the tests, to cut out some repetition.

I would have done the translation of types with a static Map and Function, and not a switch. See:

http://docs.guava-libraries.googlecode.com/git/javadoc/com/google/common/base/Functions.html#forMap(java.util.Map, V)

I don't think any of these are crucial.

@cbeer
Copy link
Contributor Author

cbeer commented Oct 28, 2013

I'll take a stab at the ZippingIterator. I thought about it and decided I wanted to get it implemented first. Going back shouldn't be too hard.

I think I've @mocked as much as I can (unless there's some annotation magic that'd let me do the extra interfaces piece.. maybe I should look harder.)

@ajs6f
Copy link
Contributor

ajs6f commented Oct 28, 2013

Sorry, @cbeer, I spoke vaguely. I didn't mean you could mock more, I meant you could mock once (as opposed to a couple places where you seem to be recreating the same mock). But I could also be misreading some of that.

Let me know if you want any help with the ZippingIterator. Just remember, it's all about a pair of iterators, one of stuff, one of functions on that kind of stuff, and what you get back is an iterator of the stuff with the functions applied.

@Component
@Scope("prototype")
@Path("/fcr:nodeTypes")
public class FedoraRepositoryNodeTypes extends AbstractResource {
Copy link

Choose a reason for hiding this comment

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

Minimally, please add a class description and @author javadoc annotation.

@ajs6f
Copy link
Contributor

ajs6f commented Oct 30, 2013

@cbeer : ItemDefinitionToTriplesTest shows multi-interface mocking via @mock. I really prefer the annotations (see @Injectmocks in that same test). But I'm okay either way. More interestingly, I don't actually see any unit tests in the kernel for the new stuff in the kernel. Did I miss that, or are you getting coverage only from the tests on HTTP endpoints?

@cbeer
Copy link
Contributor Author

cbeer commented Oct 30, 2013

@ajs6f Clever. Works for me. The stuff in kernel is covered by NodeTypeRdfContextTest (and NodeTypeIteratorTest, of course). Testing the Functions individually seemed underwhelming

@ajs6f
Copy link
Contributor

ajs6f commented Oct 30, 2013

@cbeer : Somehow, I completely failed to notice that guy (NodeTypeRdfContextTest)! Sorry. If it's okay by you, I'll do a little work with him to use the MockitoAnnotations and push that.

@cbeer
Copy link
Contributor Author

cbeer commented Oct 30, 2013

+1

@cbeer
Copy link
Contributor Author

cbeer commented Oct 31, 2013

Rebased down to the two commits for merging. Ready to go? @awoods @ajs6f ?

@awoods
Copy link

awoods commented Oct 31, 2013

@cbeer, looks like the build is failing on checkstyle violations.

awoods pushed a commit that referenced this pull request Oct 31, 2013
Translate CND to RDFS, and expose it over the REST API
@awoods awoods merged commit 55b7a00 into master Oct 31, 2013
@awoods awoods deleted the cnd-to-rdfs branch October 31, 2013 17:58
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

3 participants