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

fix: support trailing slash #120

Merged
merged 3 commits into from
Jul 17, 2024
Merged

fix: support trailing slash #120

merged 3 commits into from
Jul 17, 2024

Conversation

RafaelGSS
Copy link
Member

@RafaelGSS
Copy link
Member Author

Not sure why lint is complaining... If I remove the \/? from Fixes, tests won't pass.

@Trott
Copy link
Member

Trott commented Jul 10, 2024

Not sure why lint is complaining... If I remove the \/? from Fixes, tests won't pass.

It's complaining because you're backslash-escaping it in a string. You don't need to backslash-escape a slash character in a string, only in a regex literal.

lib/rules/fixes-url.js Outdated Show resolved Hide resolved
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Looks good to me. I assume that since you're a releaser, you've taken into consideration whether this might break relevant tools like the changelog maker. (I don't imagine it would break those tools. I'm just mentioning it in case you or someone else who deals with that tooling more often might think otherwise or want to give it a bit more consideration/testing.)

@RafaelGSS
Copy link
Member Author

I assume that since you're a releaser, you've taken into consideration whether this might break relevant tools like the changelog maker. (I don't imagine it would break those tools. I'm just mentioning it in case you or someone else who deals with that tooling more often might think otherwise or want to give it a bit more consideration/testing.)

Considering we are not dropping support but adding trailing slash support, it should not impact the existing release workflow or tools like changelog maker. But, I'll monitor it over the week.

@richardlau
Copy link
Member

I think the one possible change in behavior might be branch-diff if the PR-URL in the commits being compared differed only in whether they ended in a slash or not (since they'd no longer match).

@RafaelGSS
Copy link
Member Author

I think the one possible change in behavior might be branch-diff if the PR-URL in the commits being compared differed only in whether they ended in a slash or not (since they'd no longer match).

That's correct. Let me see this first.

@RafaelGSS
Copy link
Member Author

Created nodejs/branch-diff#70

@RafaelGSS RafaelGSS merged commit 67c75e0 into main Jul 17, 2024
3 checks passed
@richardlau richardlau deleted the support-trailing-slash branch July 17, 2024 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants