Skip to content

Conversation

@gsmet
Copy link
Member

@gsmet gsmet commented Apr 15, 2020

Copy link
Member

@yrodiere yrodiere left a comment

Choose a reason for hiding this comment

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

Looks good to me. Just a few comments about naming, but nothing critical.

Copy link
Member

Choose a reason for hiding this comment

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

annotationClassName or something like that, maybe? It's a bit hard to understand what this field stands for when reading the various methods of 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'd use getBuiltinConstraints, to make the code clearer in places where this getter is called... but it's your call.

Copy link
Member

Choose a reason for hiding this comment

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

Same here, forBuiltinConstraints? Since this won't work with custom constraints.

Copy link
Member

Choose a reason for hiding this comment

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

Some nitpicking (for a change!); I usually see this pattern written that way:

		if ( jodaTimeInClassPath == null ) {
			jodaTimeInClassPath = isClassPresent( JODA_TIME_CLASS_NAME );
		}
		return jodaTimeInClassPath.booleanValue(); 

Copy link
Member

Choose a reason for hiding this comment

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

requiredConstraints or constraintDependencies or something like that? Feel free to ignore, but I had a hard time wrapping my head around this, so a more precise name might help.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the technical term in HV but yeah I also thought about changing it to something along the lines of what you suggest so let's do it.

@gsmet
Copy link
Member Author

gsmet commented Apr 16, 2020

@yrodiere I addressed your comments.

@gsmet gsmet merged commit ea74084 into hibernate:6.1 Apr 17, 2020
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.

2 participants