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

Add option to configure via semverGit {} block #28

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

emlun
Copy link

@emlun emlun commented Jan 31, 2023

This allows the plugin to be applied with the new plugins {} DSL.

Fixes #3. Alleviates #14. Fixes #26.

Copy link
Member

@deepy deepy left a comment

Choose a reason for hiding this comment

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

I'll see if I can find something else later, but so far this looks great, just some minor things 👍

this.extension.dirtyMarker.getOrNull(),
this.extension.gitDescribeArgs.getOrNull(),
this.extension.prefix.getOrNull(),
this.project.projectDir
Copy link
Member

Choose a reason for hiding this comment

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

Since the Project is only used for projectDir can we remove project entirely?
Makes it a little easier to reason about and also doesn't prevent additional blocks for configuration-cache compatibility

Copy link
Author

Choose a reason for hiding this comment

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

Sure!

result.getOutput().contains("Version: 2.0.0-new_-NEW_DIRTYsnapshot")

where:
config = """
Copy link
Member

Choose a reason for hiding this comment

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

I like this clever use of where, unfortunately this also affects the test name giving this test the name: semverGit {} block overrides project.ext settings [config: plugins { id "com.cinnober.gradle.semver-git" apply false } ext.nextVersion = 'minor' ext.snapshotSuffix = 'LEGACYSNAPSHOT' ext.dirtyMarker = '-legacydirty' ext.gitDescribeArgs = '--match *[0-9].abc.[0-9]*.def.[0-9]*' ext.semverPrefix = 'legacy-version-' apply plugin: 'com.cinnober.gradle.semver-git' semverGit { nextVersion = 'major' snapshotSuffix = 'new_<dirty>snapshot' dirtyMarker = '-NEW_DIRTY' gitDescribeArgs = '--match *[0-9].[0-9]*.[0-9]* --long' prefix = 'new-version-' } , #0] in tools and reports 😀

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I noticed that too 😆 I experimented with a few alternatives, none of which really turned out any better:

  • Parameterize over the config Map instead of the expanded config file contents

    • This also requires an additional parameter to select which kind of config to generate. It can be embedded into the same configs() return value and setupPluginWithClasspath can "dynamic dispatch" on it, but the resulting test names aren't that much better. Doesn't seem worth the increased source code noise.
  • Parameterize over configType << ["legacy", "extension"] instead of the config Map.

    • This works well enough, but then the configType argument (an invariant across all of the tests) gets mingled in with the config Map argument (varies between tests), which entangles two orthogonal concerns. Doesn't seem worth the reduced code clarity.
  • Parameterize over the setupPluginWithClasspath function itself instead of its argument.

    • This allows the config Map to stay in the when: block instead (nice!), but makes the test names entirely incomprehensible (they show a closure type instead of the config contents).

build.gradle Show resolved Hide resolved
@deepy
Copy link
Member

deepy commented Feb 9, 2023

Alright, I found the corner-case. It's in this project's build-script 😁
The publishing section in build.gradle needs to use version.toString() otherwise the build fails.

Could you do this change and at the same time use -s on that commit to sign-off the changes?
Previously this repository required a CLA, but before I left the discussion was to replace it with DCO which is basically the same thing but with less paperwork

After that it looks good enough to be merged, I'll have to chase some people to actually get it released though so that might take some extra time

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.

Replace current ext.foo with a class nextVersion and snapshotSuffix ignored when using new plugin syntax
2 participants