Skip to content

Conversation

@yakov116
Copy link
Member

@yakov116 yakov116 commented Feb 17, 2021

@yakov116
Copy link
Member Author

My gut feeling is that we should only do this for a merged PR. I can see people having dev as the default branch and master as the release branch.

@fregante
Copy link
Member

fregante commented Feb 17, 2021

My gut feeling is that we should only do this for a merged PR

Yeah, the current code and the other feature both already have this logic, right?

There's a comment right here: aa6659a#diff-4998f02d19d02dcd71efd49bd1f943c4ac7256a392214cd0f3b7fced5d465b81R52

It should probably be brought over to this feature as well

@fregante
Copy link
Member

That made me realize. Exactly like for the previous feature, it's not just merged PRs, but also closed PRs.

Example closed PR with master: #3531

@fregante fregante marked this pull request as draft February 19, 2021 22:32
@yakov116 yakov116 marked this pull request as ready for review February 22, 2021 04:22
@fregante
Copy link
Member

fregante commented Feb 22, 2021

I think it looks good. It might be worth adding the 2 to url-detection, but isClosed should just have a single, joint selector instead of calling isMerged

 export const isMergedPR = (): boolean => exists('#partial-discussion-header [title="Status: Merged"]');
 export const isClosedPR = (): boolean => exists('#partial-discussion-header [title="Status: Closed"], #partial-discussion-header [title="Status: Merged"]');

@yakov116 yakov116 merged commit b5fdf4d into main Feb 23, 2021
@yakov116 yakov116 deleted the noise branch February 23, 2021 00:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

clean-conversation-headers should also consider master as a default

2 participants