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

a bit of a clean up for regexes #146

Closed
wants to merge 2 commits into from
Closed

Conversation

slash3b
Copy link
Contributor

@slash3b slash3b commented Aug 13, 2020

Hello!

I recently saw that comment laminas/automatic-releases#25 (comment) decided to clean class a little bit.

If I do remember correctly when TAGGED_VERSION_MATCHER regexp was compiled semver did not offer any regex recipes. So later on when they did offer one I though may be we could ditch the we have in favor of recommended.

Long story short — it does not quite what package needs 🤔

Here is the list mentioned in the comment with our regexp, do you think I should improve regular expression to better match that list ?

@@ -23,7 +23,7 @@
final class Flag
{
/**
* within extent of same version patch flag is of the highest priority
* within extent of the same version "patch" flag is of the highest priority
Copy link
Member

Choose a reason for hiding this comment

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

This reads very clunky 😬

* - stability flag, e.g. alpha, beta and etc.
* - stability numbers
*/
public const TAGGED_VERSION_MATCHER = '\s*(?<version>(?:\d+\.)*\d+)(?:-(?<flag>stable|beta|b|rc|alpha|a|patch|p)[._-]?(?<stability_numbers>(?:\d+\.)*\d+)?)?\s*';
Copy link
Member

Choose a reason for hiding this comment

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

Is there any functional change in this patch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no functional change in this patch, just wanted it to be a bit more clean.

Copy link
Member

Choose a reason for hiding this comment

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

I must say that I find the previous version cleaner/easier to follow :|

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay, then lets close it. No worries.
Sorry for the noise ☺️

Copy link
Member

Choose a reason for hiding this comment

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

Alright, thanks for the effort anyway!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants