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

Let transforms consider mixin types #214

Merged
merged 1 commit into from Dec 15, 2013
Merged

Let transforms consider mixin types #214

merged 1 commit into from Dec 15, 2013

Conversation

ajs6f
Copy link
Contributor

@ajs6f ajs6f commented Dec 15, 2013

@ghost ghost assigned cbeer Dec 15, 2013
final NodeType primaryNodeType = node.getPrimaryNodeType();

final NodeType[] supertypes = primaryNodeType.getSupertypes();

final Iterable<NodeType> nodeTypes =
concat(of(primaryNodeType), copyOf(supertypes));
final Set<NodeType> nodeTypes =
Copy link
Contributor

Choose a reason for hiding this comment

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

What sort of ordering guarantee do we get here? At the very least, are the results deterministic? I'm not sure how we ought to balance the priorities of primary vs mixin types, but we should at least have consistent behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're not guaranteeing right now, and that was okay for my simple tests. I'll change to a List to guarantee mixins first, then primary type, then primary's supertypes. That would be best for the indexing workflow, which is the chief use case for fcr:transform right now.

Do you think I should order mixins by name and supertypes by name, just to get more predictability? Do you think we should include the supertypes of mixin's (which are formally types of the node as well, just as the supertypes of the primary are)? I think the answer to both questions is yes, but I'll defer to you.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. That seems good enough for now, I think. There's plenty of time to be clever later.

cbeer added a commit that referenced this pull request Dec 15, 2013
Let transforms consider mixin types
@cbeer cbeer merged commit 83ac74e into master Dec 15, 2013
@cbeer cbeer deleted the MixinsForTransforms branch December 15, 2013 17:14
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

2 participants