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 include-pre-releases
configuration option
#1302
Conversation
@jetersen ready for review :) |
exclude-pre-releases
configuration option
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two maintainability considerations, but lgtm other than that
lib/releases.js
Outdated
@@ -57,7 +58,9 @@ const findReleases = async ({ | |||
? commitishFilteredReleases.filter((r) => r.tag_name.startsWith(tagPrefix)) | |||
: commitishFilteredReleases | |||
const sortedPublishedReleases = sortReleases( | |||
filteredReleases.filter((r) => !r.draft && !r.prerelease) | |||
filteredReleases.filter( | |||
(r) => !r.draft && (!excludePreReleases || !r.prerelease) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logic checks out, but it doesn't read right.
!r.draft && (!r.prerelease || includePreReleases)
would be preferable.
I'm quite concerned about the amount of boolean logic in this function already which is why I'm even giving this a comment. It probably needs to get reflowed entirely, but that's out of scope here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed the configuration to include-pre-releases
which makes more sense :) Thanks for your input!
exclude-pre-releases
configuration optioninclude-pre-releases
configuration option
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thank you!
@jetersen sorry to poke you, but can you have a look at this PR and decide whether or not you want this at all? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
include-pre-releases
configuration optioninclude-pre-releases
configuration option
@robbinjanssen Thanks for the PR, sorry for not getting back to you sooner. |
The change introduced in #1240 broke the release flow for some people. By adding a configuration option to
include-pre-releases
we can make this work for everybody :-) The default isfalse
so the behaviour introduced in #1240 will be the default behaviour.tl;dr - If you want to allow pre-releases to count as releases, add
include-pre-releases: true
to your configCloses #1291
Closes #1293
When merging, please squash :-)