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

feat: use branch prerelease property for detection #863

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

klopfdreh
Copy link

@klopfdreh klopfdreh commented Jun 24, 2024

For the description have a look into the issue: #864

For a showcase have a look at the video provided by @babblebey in the comment: #863 (comment)

Fixes #864

lib/is-prerelease.js Outdated Show resolved Hide resolved
Copy link
Member

@babblebey babblebey left a comment

Choose a reason for hiding this comment

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

Great stuff @klopfdreh,

Thanks for the test, super valuable 😉, kindly address the following..

  • The suggested changes
  • The Test failing in CI: Do address the lint issues, by running the command npm run lint:prettier:fix on your dev branch... then run the tests again see whether it passes

I think this PR is almost good to go 👍

On other note:

I have edited your PR description to properly link your Issue to the PR, this style of linking Issues to PR help automatically closes an Issue once the linked PR gets merged, kindly take note of this approach 😉

@klopfdreh klopfdreh force-pushed the feature/prereleasedetection branch from 12b9a4f to 4e47bee Compare June 26, 2024 19:39
@klopfdreh
Copy link
Author

Great stuff @klopfdreh,

Thanks for the test, super valuable 😉, kindly address the following..

  • The suggested changes
  • The Test failing in CI: Do address the lint issues, by running the command npm run lint:prettier:fix on your dev branch... then run the tests again see whether it passes

I think this PR is almost good to go 👍

On other note:

I have edited your PR description to properly link your Issue to the PR, this style of linking Issues to PR help automatically closes an Issue once the linked PR gets merged, kindly take note of this approach 😉

Thanks a lot for all the hints. I force pushed the changes after using npm run lint:prettier:fix - I think all lint issues should be fixed, now.

@babblebey
Copy link
Member

Thanks a lot for all the hints. I force pushed the changes after using npm run lint:prettier:fix - I think all lint issues should be fixed, now.

Great stuff @klopfdreh,

This is set... but d'you know one thing that can make a PR look even good!? 😉 A screen recording of a demo where you run the changes to show it really works as expected 😉, this is super valuable for Reviewers 👨‍💻 if it were to be present in the PR description❤️

I have tested it and below is a screencast demoing your change for the attention of @semantic-release/maintainers 🤝

screencast-bpconcjcammlapcogcnnelfmaeghhagj-2024.06.26-21_20_37.webm

@babblebey babblebey requested a review from a team June 26, 2024 20:35
@klopfdreh
Copy link
Author

I am going to link the showcase in the PR description - thanks for the advice 👍 - next time I am going to also provide a screen recording.

@klopfdreh klopfdreh requested a review from babblebey June 26, 2024 20:59
Copy link
Member

@gr2m gr2m 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, just a tiny nit

lib/is-prerelease.js Outdated Show resolved Hide resolved
Co-authored-by: Gregor Martynus <[email protected]>
@klopfdreh
Copy link
Author

Great - thanks a lot for having a look at it! 👍

Anything I have to do here or is this PR ready for merge? 😃

@gr2m
Copy link
Member

gr2m commented Jul 1, 2024

I'll let @babblebey take it from here 👍🏼

Copy link
Member

@babblebey babblebey left a comment

Choose a reason for hiding this comment

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

Let's goooo 💪🏾

@klopfdreh
Copy link
Author

Let's goooo 💪🏾

💯 Let‘s gooo 😄

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.

Improvement of prerelease check
3 participants