Skip to content

Conversation

@JonasPammer
Copy link
Contributor

Resolves apache/grails-core#14329

Commit Message out-take:

README.md text was copied with template from https://github.com/spockframework/spock/blob/master/docs/extensions.adoc#global-extensions

I tried to get knowledge about this ServiceLoader paradigm from grails-core code search and oracle doc

Although this whole META-INF is weird to me,
I did not find it necessary to implement something complex like spock's ConfigurationScriptLoader, which uses (try to load env var path, file system, ..) into this just for this.

PR Appendix to last paragraph: Maybe the first one (env var) seems like a viable alternative/addition to this pattern, given that all other settings can be managed through systemProperty 'grails.geb.recording.* '*'``, i did not roll with using it instead as to first try to do it as per agreed upon https://github.com/grails/geb/issues/133#issuecomment-2654122264, and to possibly get validation if i did the ServiceLoader Pattern right ❤️ 💭

@CLAassistant
Copy link

CLAassistant commented Feb 21, 2025

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@matrei matrei left a comment

Choose a reason for hiding this comment

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

Awesome @JonasPammer, thank you for this PR!

I do have some concern for the tests, in that now all tests will use the custom implementation, and no tests will use the default implementation. If we create and use a TestDescriptionServiceFactory and call that instead of the ServiceLoader directly, we can modify the implementation programatically in setupSpec() and cleanupSpec().

Also, I think we should target the 4.2.x branch with this PR as we have not yet released a 4.2.0 version.

What do you think?

@JonasPammer
Copy link
Contributor Author

Totally! I saw other 2 projects in grails/spock also use a seperate class (i think there called loader), will implement thanks to your helpful guiding comments.

@JonasPammer JonasPammer changed the base branch from 5.0.x to 4.2.x February 22, 2025 16:49
@JonasPammer JonasPammer force-pushed the geb-133-serviceloader branch from b2076f6 to 8186a9b Compare February 22, 2025 16:50
@JonasPammer
Copy link
Contributor Author

Force Push Explanation

[changed pr base branch](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/changing-the-base-branch-of-a-pull-request)

~\IdeaProjects\geb git:[geb-133-serviceloader]
git fetch upstream 4.2.x
From https://github.com/grails/geb
 * branch            4.2.x      -> FETCH_HEAD
~\IdeaProjects\geb git:[geb-133-serviceloader]
git fetch upstream 5.0.x
From https://github.com/grails/geb
 * branch            5.0.x      -> FETCH_HEAD
~\IdeaProjects\geb git:[geb-133-serviceloader]
git rebase --onto upstream/4.2.x upstream/5.0.x
Successfully rebased and updated refs/heads/geb-133-serviceloader.
~\IdeaProjects\geb git:[geb-133-serviceloader]
git push -f origin geb-133-serviceloader
Enumerating objects: 65, done.
Counting objects: 100% (65/65), done.
Delta compression using up to 12 threads
Compressing objects: 100% (30/30), done.
Writing objects: 100% (47/47), 5.58 KiB | 815.00 KiB/s, done.
Total 47 (delta 20), reused 0 (delta 0), pack-reused 0 (from 0)
remote: Resolving deltas: 100% (20/20), completed with 7 local objects.
To https://github.com/JonasPammer/geb.git
 + b2076f6...8186a9b geb-133-serviceloader -> geb-133-serviceloader (forced update)

@JonasPammer
Copy link
Contributor Author

This PR (and, also seeing you there ;) gets me to also maybe open a PR in grails-docs for minor fixes found along the way, its fun

i am happy to drop some commits if desired, e.g. the last one - [add ContainerGebConfiguration ability](a9045bf)

@JonasPammer JonasPammer requested a review from matrei February 22, 2025 18:33
@matrei matrei changed the title refactor: customizeable recording file name feat: customizeable recording file name Feb 22, 2025
@jdaugherty
Copy link
Contributor

@JonasPammer Thank you for taking the time on this. I had only minor comments / cleanup.

Copy link
Contributor

@matrei matrei left a comment

Choose a reason for hiding this comment

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

Excellent! I have some minor suggestions/questions.

@JonasPammer
Copy link
Contributor Author

thanks for helping me improve, will implement changes sometime tomorrow

@JonasPammer
Copy link
Contributor Author

left 2 comments open ended to either be resolved or answered with that we should do it the way written

@JonasPammer JonasPammer requested a review from matrei February 25, 2025 20:50
Copy link
Contributor

@matrei matrei left a comment

Choose a reason for hiding this comment

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

Nice! Just some follow up comments/suggestions.

@JonasPammer
Copy link
Contributor Author

👍

@JonasPammer JonasPammer requested a review from matrei February 26, 2025 20:47
@JonasPammer
Copy link
Contributor Author

last commit can be dropped, i wanted to hold a grail to the actual code of your alls which this plugin is about to not be bloated by java design patterns necessities of 4 classes

@matrei
Copy link
Contributor

matrei commented Feb 27, 2025

Thanks @JonasPammer, I think we can drop the last commit for now (it might actually be a breaking change in 4.x).
After that I'm happy with this PR and will gladly approve.

@JonasPammer JonasPammer force-pushed the geb-133-serviceloader branch from 058d3b2 to 3fceea4 Compare February 27, 2025 06:35
@matrei
Copy link
Contributor

matrei commented Feb 27, 2025

Talking of breaking changes. Changing ContainerGebTestDescription to an interface is actually a breaking change. @jdaugherty What do you think, should we target 5.0.x with this PR instead?

@JonasPammer
Copy link
Contributor Author

if its just about the namespacing, i could make ContainerGebTestDescription be the current Default...., and transform the interface class name to IContainer..., if the I pattern is ok

@jdaugherty
Copy link
Contributor

Talking of breaking changes. Changing ContainerGebTestDescription to an interface is actually a breaking change. @jdaugherty What do you think, should we target 5.0.x with this PR instead?

It was internal only so I think it's ok in this case.

@JonasPammer
Copy link
Contributor Author

given image i will go ahead with the Default.. Naming scheme in #146

@JonasPammer
Copy link
Contributor Author

set do draft as per above comment linked PR design decision WIP

README.md text was copied with template from https://github.com/spockframework/spock/blob/master/docs/extensions.adoc#global-extensions

I tried to get knowledge about this ServiceLoader paradigm from https://github.com/search?q=org%3Agrails+repo%3Agrails%2Fgrails-core+ServiceLoader.load&type=code and https://docs.oracle.com/javase/8/docs/api/java/util/ServiceLoader.html class javadoc

Although this whole META-INF is weird to me,
I did not find it necessary to implement something complex like https://github.com/spockframework/spock/blob/620f8734fb51c26cc9c8b8aade8d4dedc1532b94/spock-core/src/main/java/org/spockframework/runtime/ConfigurationScriptLoader.java#L34 (try to load env var path, file system, ..) into this just for this

# Conflicts:
#	README.md
"left-shift cannot be applied to ..."
commit/file/folder can be removed, as documentation is clear
although default doesnt use it, as to not change default behaviour with this PR, one may want to?
intellij refactor + Spec needed manually
@JonasPammer JonasPammer force-pushed the geb-133-serviceloader branch from d336f0b to 41b4590 Compare April 20, 2025 06:30
@JonasPammer JonasPammer marked this pull request as ready for review April 20, 2025 06:59
@JonasPammer
Copy link
Contributor Author

as per apache/grails-core#14330 i understand when this will be closed i will reopen a new version in core

@jdaugherty
Copy link
Contributor

@JonasPammer you should be safe to open this against grails core now.

grails-geb is the source location in core
grails-test-examples/geb is the test app

@jdaugherty jdaugherty closed this Apr 22, 2025
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.

feature request and quick fix: provide an extension point for customizing the file name

4 participants