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

HHH-13884 Order.reverse() contract #3272

Merged

Conversation

seregamorph
Copy link
Contributor

@seregamorph seregamorph commented Feb 28, 2020

https://hibernate.atlassian.net/browse/HHH-13884

backported in #3271

javax.persistence.criteria.Order.reverse() method javadoc explicitly declares that reverse() should return new instance.

private SqmExpression sortExpression;
private String collation;
private SortOrder sortOrder;
private final NullPrecedence nullPrecedence;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

please note: nullPrecenence duplicates precedence field.
nullPrecendence was assigned only in constructor and never used, all other access is done to precedence field.
I looks like a defect. I fixed it, because otherwise I could not write a test to validate all assignments in this class.

Copy link
Member

Choose a reason for hiding this comment

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

I prefer the name nullPrecedence - its more consistent and self-documenting

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Copy link
Member

@sebersole sebersole left a comment

Choose a reason for hiding this comment

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

Minor request to change field name. Other than that - +1

private SqmExpression sortExpression;
private String collation;
private SortOrder sortOrder;
private final NullPrecedence nullPrecedence;
Copy link
Member

Choose a reason for hiding this comment

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

I prefer the name nullPrecedence - its more consistent and self-documenting

@seregamorph
Copy link
Contributor Author

@sebersole requested change addressed. Still any objections to merge it?

@seregamorph
Copy link
Contributor Author

@sebersole @beikov please make a decision on it. It was created half a year ago. If you don't like it, lets close.

@beikov
Copy link
Contributor

beikov commented Sep 28, 2020

Could you please rebase it, I can't merge it right now.

@seregamorph seregamorph force-pushed the HHH-13884-6.0-order-reverse-contract branch from 7fa3416 to 22def92 Compare September 28, 2020 07:17
@seregamorph
Copy link
Contributor Author

@beikov rebased on top of wip/6.0 and squashed into a single commit without changes.

@beikov beikov merged commit eafd262 into hibernate:wip/6.0 Sep 28, 2020
@seregamorph seregamorph deleted the HHH-13884-6.0-order-reverse-contract branch September 28, 2020 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants