Skip to content

Conversation

marko-bekhta
Copy link
Member

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 suggestion. Looks good.

import org.gradle.api.tasks.TaskAction;

/**
* Responsible for coordinating the actual injection of project version. Runs as a Gradle Action (doLast typically
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Responsible for coordinating the actual injection of project version. Runs as a Gradle Action (doLast typically
* Responsible for coordinating the actual injection of project version.

public abstract Property<String> getVersion();

@Input
public abstract ListProperty<TargetMember> getInjectionTargets();
Copy link
Member Author

Choose a reason for hiding this comment

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

BTW, I've tried adding an output, as:

	@OutputDirectory
	public FileCollection getOutputFiles() {
		return getClasspath();
	}

^ my thinking was since we update classes, it somewhat makes sense ...

but I had build issues on the ORM side when I tried building an entire project, rather than just a single module where the injection plugin is actually applied. Some of those modules that didn't need the plugin were complaining...

maybe it's because of that "global" declaration here:

https://github.com/hibernate/hibernate-orm/blob/0080e17b43515f4a6e12218dd16dc879659e64a0/build.gradle#L22

🤔

Copy link

@kingg22 kingg22 left a comment

Choose a reason for hiding this comment

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

Hey, can you remove this empty constructor is not needed? foresee future use of that project parameter. Or rollback to instance with empty properties and maintain the previous DSL, idk because don't have test here.

@marko-bekhta
Copy link
Member Author

Hey, can you remove this empty constructor is not needed? foresee future use of that project parameter. Or rollback to instance with empty properties and maintain the previous DSL, idk because don't have test here.

I'll assume you were talking about the InjectionSpec here 🙂

@kingg22
Copy link

kingg22 commented Oct 17, 2025

Hey, can you remove this empty constructor is not needed? foresee future use of that project parameter. Or rollback to instance with empty properties and maintain the previous DSL, idk because don't have test here.

I'll assume you were talking about the InjectionSpec here 🙂

Yes, GH don't link the lines.
You didn't restore the DSL. Will you apply the change to the ORM? Wouldn't it be easier to keep the INTO function? IDK, I'll leave it up to you.
Anyways, LGTM, I'm waiting to review in my branch before send a PR to ORM.

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