github icon indicating copy to clipboard operation
github copied to clipboard

feat: Add githubReleaseAssets option

Open esatterwhite opened this issue 1 year ago • 4 comments

Includes the addition of a new option githubReleaseAssets to replace the old assets option. The assets option is identical to the option found in the git plugin but is ambiguous when used in shared configuration packages.

Fixes: #808

esatterwhite avatar Apr 05 '24 17:04 esatterwhite

Having seen your issue https://github.com/semantic-release/github/issues/808, I do not subscribe to introducing a new config property githubReleaseAssets as the fix for the issue for the following reasons..

  • replacing assets with githubReleaseAssets will be BREAKING CHANGE (avoiding that as much as possible)
  • it removes the simplicity from the naming and makes our available plugin apis less uniform
  • the eco-system impact: imagine needing to make similar changes with the gitlab/bitbucket and every hosted git platform plugins too

Thinking real loud here, I believe we can do better by leaning towards a priority styles solution where the level of definition of the assets property determines how its set 🤔

babblebey avatar Aug 12 '24 19:08 babblebey

  • replacing assets with githubReleaseAssets will be BREAKING CHANGE (avoiding that as much as possible)

This still uses assets if githubReleaseAssets is not set. So it doesn't break anything.

  • it removes the simplicity from the naming and makes our available plugin apis less uniform

ok, so we rename it releaseAssets (or something similarly generic) rather than specifically githubReleaseAssets. That is as simple as githubApiPathPrefix. Doing so will allow the config options to be the same across different plugins.

To be fair, the simplicity in naming here is the thing that makes it difficult to use. Which is the more important thing

  • the eco-system impact: imagine needing to make similar changes with the gitlab/bitbucket and every hosted git platform plugins too

I think the above change addresses this.

Thinking real loud here, I believe we can do better by leaning towards a priority styles solution where the level of definition of the assets property determines how its set 🤔

That would also a breaking change. a default will have to be put in place, and it more than likely will not match what people have in place. I additionally postulate that breaks the simplicity ideal. It also means that other plugins will have to follow suite. It is not a common pattern in the semantic-release eco-system.

esatterwhite avatar Aug 12 '24 21:08 esatterwhite

That would also a breaking change. a default will have to be put in place....

This is the way the config should just operate... when there's an assets property defined within the plugin property or extends then it should be prioritized over any other assets definition in the scope above it.

I can easily assume that this is how the package currently functions, but considering that it isn't (I still want verify this behavior), we gotta just fix exactly that 🤔

Let's continue this discussion in https://github.com/semantic-release/github/issues/808

babblebey avatar Aug 13 '24 16:08 babblebey

This is the way the config should just operate... when there's an assets property defined within the plugin property or extends then it should be prioritized over any other assets definition in the scope above it

The problem isn't priority of resolution. the problem is that a single key in a json object cannot represent 2 different settings

This is the way the config should just operate...

I don't disagree, My point here is that the argument against a breaking change isn't really relevant, as it would potentially be a breaking change in either case.

esatterwhite avatar Aug 13 '24 18:08 esatterwhite