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 ReleaseNotes SemVer #2686

Merged

Conversation

Freymaurer
Copy link
Contributor

Description

@aggieben, @yazeedobaid

Had to make SemVer parsing a bit more complicated, but should still be clearly understandable. I also added comments to explain certain choices.

@Freymaurer Freymaurer changed the title Fix release notes sem ver Fix ReleaseNotes SemVer Jul 18, 2022
let nugetRegexLegacy = String.getRegEx @"([0-9]+.)+[0-9]+(-[a-zA-Z]+\d*)?(.[0-9]+)?"
let nugetRegex =
/// From Fake.Core.SemVer
let pattern =
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can u please re-use patterns from Fake.Core.SemVer instead of duplicating them here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can i pull pattern out of (|SemVer|_|) to use it? Like such?

let pattern =
      @"^(?<major>\d+)" +
      @"(\.(?<minor>\d+))?" +
      @"(\.(?<patch>\d+))?" +
      @"(\-(?<pre>[0-9A-Za-z\-\.]+))?" +
      @"(\+(?<build>[0-9A-Za-z\-\.]+))?$"

let (|SemVer|_|) version =
        match version with
        | ParseRegex pattern [major; minor; patch; pre; build] ->
            Some [major; minor; patch; pre; build]
        | _ ->
            None

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah sure

Copy link
Collaborator

@yazeedobaid yazeedobaid left a comment

Choose a reason for hiding this comment

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

Review changes

@yazeedobaid yazeedobaid added the in-review Convey that a PR is in-review status label Jul 21, 2022
@Freymaurer
Copy link
Contributor Author

Done @yazeedobaid 👍

@yazeedobaid
Copy link
Collaborator

Thanks a lot.
One last request, can u please remove the changes in Fake.sln file from PR to make the PR focused on issue at hand?

@Freymaurer
Copy link
Contributor Author

Done @yazeedobaid 👍 Hope it worked out.

Copy link
Collaborator

@yazeedobaid yazeedobaid left a comment

Choose a reason for hiding this comment

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

Thanks a lot

@yazeedobaid yazeedobaid removed the in-review Convey that a PR is in-review status label Jul 21, 2022
@yazeedobaid yazeedobaid merged commit 3995ed2 into fsprojects:release/next Jul 21, 2022
@aggieben
Copy link

🎉 Thanks @Freymaurer !

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.

ReleaseNotes.load does not parse SemVer correctly
3 participants