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

F: Only register version for the current project #97

Merged
merged 1 commit into from
Oct 24, 2022

Conversation

ericmaino
Copy link

The main challenge is that currently the plugin is partially registering it self with subprojects, but not fully doing so. It should be updated to fully register and handle versioning everywhere or only do it for the project configured. This change moves it to only register for the currently configured project.

I thought of making the existing behavior conditional, however even then it didn't feel like the correct change.

The main challenge is that currently the plugin is partially registering it self with
subprojects, but not fully doing so. It should be updated to fully register and
handle versioning everywhere or only do it for the project configured.
This change moves it to only register for the currently configured project.

I thought of making the existing behavior conditional, however
even then it didn't feel like the correct change.
@ericmaino
Copy link
Author

Split from original PR #94

@qoomon qoomon self-requested a review October 18, 2022 11:20
@qoomon qoomon self-assigned this Oct 18, 2022
@qoomon qoomon removed their request for review October 18, 2022 11:20
@qoomon qoomon added the enhancement New feature or request label Oct 18, 2022
@qoomon
Copy link
Owner

qoomon commented Oct 18, 2022

Maybe this task should be moved to a separate plugin. WDYT?

@ericmaino
Copy link
Author

I think we should register it as it's useful. The correct fix should also be to categorize and add a description. We could also conditionally register it. It's useful to have a built in task to check the version.

// on
// sub projects. It would be nice to only define this once for all project in
// a multi-module project, however there are a few other considerations that
// need
Copy link
Owner

Choose a reason for hiding this comment

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

I think we need to describe the problem to solve ("...other considerations that need to be made...").

Copy link
Author

Choose a reason for hiding this comment

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

The current change scopes the plugin to the project in which is registered. This is the common pattern for most plugins. There are a few paths that could be followed.

  1. Leave as is where a version task is registered everywhere. I personally don't see the value in only registering part of the plugin. This registers the task, but does not apply the configuration aspects. If this is choose, the task should be registered conditionally and only afterEvaulate has completed. This would still allow the plug to be registered on other projects, which today it fails
  2. Take the change as is only register the plugin and it's tasks in the project requested. This is the pattern most plugins follow. It's the safest and easiest to get correct.
  3. Register and apply not only the version task, but also the versioning semantics to every project when in a multi-project. The challenge with this is that since the versioning happens during the configuration phase ordering is very important and getting this right will be difficult

Copy link
Author

Choose a reason for hiding this comment

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

I would consider this a bug and not an enhancement, since as the plugin is currently authored it is unusable in a multi-project structure.

@qoomon qoomon merged commit a8fbfca into qoomon:master Oct 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants