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 a collapse-after option for categories #1095

Merged
merged 14 commits into from Mar 7, 2022
Merged

Add a collapse-after option for categories #1095

merged 14 commits into from Mar 7, 2022

Conversation

robbinjanssen
Copy link
Contributor

@robbinjanssen robbinjanssen commented Mar 4, 2022

This implements a new feature as discussed in #1093

Add the collapse-after integer to a category to make it hide the PRs if there are more than the given int.

@jetersen
Copy link
Member

jetersen commented Mar 4, 2022

@robbinjanssen Please use yarn install and yarn prettier && yarn lint --fix && yarn build

using npm causes these large changeset in dist/index.js

lib/releases.js Outdated

if (
!category.collapse ||
(category.collapse && category.pullRequests.length < 4)
Copy link
Member

Choose a reason for hiding this comment

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

If you think this needs to be configurable, feel free to add it :)

Please follow the current config naming with word-another-word

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could, but I have no idea what to name it 🙈 Any suggestions?

Copy link
Member

@jetersen jetersen Mar 4, 2022

Choose a reason for hiding this comment

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

collapse-at is fine it will go together with collapse. The readme would have to document whether it is greater than or less than that is used.
The default may be greater than 3?

Potentially could move the condition to outside the if statement.

So the if statement might say if (shouldCollapse) or if (shouldNotCollapse)

lib/releases.js Outdated Show resolved Hide resolved
lib/releases.js Show resolved Hide resolved
Co-authored-by: Joseph Petersen <josephp90@gmail.com>
@jetersen
Copy link
Member

jetersen commented Mar 4, 2022

@rnorth @bsideup @kiview This might be interesting for you :) I see you used details on release notes for https://github.com/testcontainers/testcontainers-java

If you have any input that would be appreciated 😅

@rnorth
Copy link
Contributor

rnorth commented Mar 4, 2022

Wow this is great! I've been doing this by hand and it's such a pain. Thank you!

The only input I'd offer would be around collapse-at. I definitely think this value being configurable makes a lot of sense.

I wonder if having both collapse and collapse-at is potentially confusing though. The behaviour can differ depending on whether one/both/other are set, and the user has to mentally internalise some boolean logic to know what will happen.

Would it be too radical to suggest having only collapse-at?

If not set, the behaviour would be to never collapse (use MAX_VALUE as the default). If set, collapse would be implicitly true when the length exceeds the value.

@jetersen
Copy link
Member

jetersen commented Mar 4, 2022

Only having collapse-at makes a lot of sense 😄

Instead of using MAX_VALUE I suggest simply conditional based on whether collapse-at is 0. When zero do nothing, makes sense as 0 is the default value for an integer.

const shouldCollapse = collapseAt != 0 && category.pullRequests.length > collapseAt

whether it is >= or > I am not sure 😅

@robbinjanssen
Copy link
Contributor Author

Great suggestions! collapse-at definitely makes sense, i'll try to update the pr as soon as i got some spare time!

@jetersen
Copy link
Member

jetersen commented Mar 6, 2022

one more suggestion, perhaps collapse-after makes more sense :) So collapse-after: 3 would mean pullRequests.length > 3 would mean it collapses once it hits 4 😅

@robbinjanssen robbinjanssen changed the title Add a collapse option for categories Add a collapse-after option for categories Mar 6, 2022
@robbinjanssen
Copy link
Contributor Author

@jetersen done! Let me know if you want any more changes :-)

lib/schema.js Outdated Show resolved Hide resolved
lib/releases.js Outdated Show resolved Hide resolved
@jetersen
Copy link
Member

jetersen commented Mar 6, 2022

41b8bf1 great addition 😄

@jetersen
Copy link
Member

jetersen commented Mar 6, 2022

@rnorth do you mind reviewing it?

@jetersen jetersen added the type: feature New feature or request label Mar 6, 2022
@jetersen
Copy link
Member

jetersen commented Mar 7, 2022

@rnorth any concerns or do you think I can go ahead and merge?

If your too busy just let me know 😅

@rnorth
Copy link
Contributor

rnorth commented Mar 7, 2022

@rnorth any concerns or do you think I can go ahead and merge?

If your too busy just let me know 😅

Sorry I'm off sick at the moment. I'm sure if you've reviewed then it's going to be fine. Thank you!

@jetersen
Copy link
Member

jetersen commented Mar 7, 2022

@rnorth thanks and get well.

@jetersen jetersen merged commit 3cbe8cc into release-drafter:master Mar 7, 2022
@jetersen
Copy link
Member

jetersen commented Mar 7, 2022

@robbinjanssen thank you for your contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants