Skip to content

Conversation

@quaff
Copy link
Contributor

@quaff quaff commented Dec 17, 2024


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license
and can be relicensed under the terms of the LGPL v2.1 license in the future at the maintainers' discretion.
For more information on licensing, please check here.


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

@quaff
Copy link
Contributor Author

quaff commented Dec 17, 2024

@gavinking Please review.

* @return the Map of annotation attributes, with attribute names as keys and
* corresponding attribute values as values (never {@code null})
*/
public static Map<String, Object> getAttributes( Annotation annotation ) {
Copy link
Member

Choose a reason for hiding this comment

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

Map<String,Object> is what we've just spent years trying to get away from.

@gavinking
Copy link
Member

So I think this patch is in some sense perfectly fine and reasonable, but the thing is that Configurable, like the rest of the package it belongs to, is old stuff that we've already moved away from (except for the built-in generators). I don't think we should be giving it a second lease on life.

We moved to the new approach in order to achieve type safety, and this change undermines that.

So I guess I'm not really very keen on this.

@quaff
Copy link
Contributor Author

quaff commented Dec 17, 2024

We moved to the new approach in order to achieve type safety, and this change undermines that.

How about introduce a new functional interface accept the annotation before configure?

@beikov
Copy link
Member

beikov commented Dec 17, 2024

Please create PRs primarily against the main branch, especially if it's an improvement or new feature.

@quaff
Copy link
Contributor Author

quaff commented Dec 18, 2024

Please create PRs primarily against the main branch, especially if it's an improvement or new feature.

OK, should I create another PR or modify this one? I'd like it's fixed in 6.6.

@gavinking
Copy link
Member

I'd like it's fixed in 6.6.

That's definitely not going to happen. 6.6 is finished and done.

But I'm not even convinced we should do this at all.

@quaff
Copy link
Contributor Author

quaff commented Dec 19, 2024

Alternatively, we could put the annotation self into parameters instead of its attributes.

@quaff quaff changed the base branch from 6.6 to main December 19, 2024 08:52
@gavinking
Copy link
Member

Alternatively, we could put the annotation self into parameters instead of its attributes.

That's certainly better.

@gavinking
Copy link
Member

Well,

Alternatively, we could put the annotation self into parameters instead of its attributes.

That's certainly better.

Well, I dunno ... maybe not. It's somewhat more typesafe, at least.

@gavinking
Copy link
Member

How about introduce a new functional interface accept the annotation before configure?

I think if we were going to do this, this would be the way, because the interface would be parameterized by the annotation type, and it would be type safe. But it means adding an additional interface and additional complexity for something that I'm just not convinced we have a great justification for.

@gavinking
Copy link
Member

How about introduce a new functional interface accept the annotation before configure?

I think if we were going to do this, this would be the way, because the interface would be parameterized by the annotation type, and it would be type safe. But it means adding an additional interface and additional complexity for something that I'm just not convinced we have a great justification for.

Actually @quaff, I've been forgetting something. We already have an interface like this, and from looking at the code its initialize() method already gets called after a Generator is instantiated via the BeanContainer.

So I believe the functionality you're asking for already exists (and is even documented), I had just forgotten about it because I'm swapped-out on this stuff.

@quaff
Copy link
Contributor Author

quaff commented Dec 23, 2024

How about introduce a new functional interface accept the annotation before configure?

I think if we were going to do this, this would be the way, because the interface would be parameterized by the annotation type, and it would be type safe. But it means adding an additional interface and additional complexity for something that I'm just not convinced we have a great justification for.

Actually @quaff, I've been forgetting something. We already have an interface like this, and from looking at the code its initialize() method already gets called after a Generator is instantiated via the BeanContainer.

So I believe the functionality you're asking for already exists (and is even documented), I had just forgotten about it because I'm swapped-out on this stuff.

My bad, you are right.

@quaff quaff closed this Dec 23, 2024
@quaff
Copy link
Contributor Author

quaff commented Dec 23, 2024

and is even documented

@gavinking The document mentioned AnnotationBasedGenerator, it should elaborate for such user case.

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