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

version: improve url-only version parsing #2827

Merged
merged 2 commits into from Jun 30, 2017

Conversation

Projects
None yet
2 participants
@JCount
Copy link
Contributor

JCount commented Jun 26, 2017

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew tests with your changes locally?

add support for both styles prefixed with up to two characters,
and styles with four groups of digits, seperated by periods

I believe I made the regex sufficiently strict to prevent unwanted results, but if necessary it could be split into two or more pieces. Some of the regex syntax itself is also more esoteric so it may be desirable to modify them for that reason as well.

Originally begun as a fix for homebrew/core#14957.

I added tests, which is good, but did I add too many?

cc @ilovezfs @dunn

@JCount JCount force-pushed the JCount:version-improve-url-only-version-parsing branch from b097534 to a7251a9 Jun 26, 2017

# e.g. https://wwwlehre.dhbw-stuttgart.de/~sschulz/WORK/E_DOWNLOAD/V_1.9/E.tgz
# e.g. https://github.com/JustArchi/ArchiSteamFarm/releases/download/2.3.2.0/ASF.zip
# e.g. https://people.gnome.org/~newren/eg/download/1.7.5.2/eg
m = %r{/[a-z_]{,2}(\d\.\d+(\.\d+){,2})}i.match(spec_s)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jun 27, 2017

Member

What's the [a-z_]{,2} and (.\d+){,2} handling?

This comment has been minimized.

@JCount

JCount Jun 27, 2017

Contributor

@MikeMcQuaid My attempted explanation follows:

  • The [a-z_]{,2}, combined with the case-insensitive modifier, will handle cases where the version substring in the url is prefixed by a combination of zero to two letters or underscores. This will handle, from the examples I've given, /releases/download/r1.9.293/cljs.jar, /download/v0.6.1/fullsrc.zip, and /WORK/E_DOWNLOAD/V_1.9/E.tgz.

  • (\.\d+){,2} will handle instances where there are, in addition to the two, . separated base groups of digits, an additional one to two . prefixed digit groupings concatenated onto that base, for a total of up to four groupings of digits separated by .s. This will handle, for example, /download/2.3.2.0/ASF.zip and /download/1.7.5.2/eg, while not breaking the new handling of /WORK/E_DOWNLOAD/V_1.9/E.tgz.

I apologize for the awkward language, it is difficult to turn regex into understandable language, 🤔.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jun 29, 2017

Member

will handle cases where the version substring in the url is prefixed by a combination of zero to two letters or underscores

I'd make this more specific e.g. [rvV]_?

will handle instances where there are, in addition to the two

This one makes sense 👍

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jun 27, 2017

Nice work @JCount. One question on this.

@JCount JCount force-pushed the JCount:version-improve-url-only-version-parsing branch from a7251a9 to e77adf3 Jun 29, 2017

@JCount

This comment has been minimized.

Copy link
Contributor

JCount commented Jun 29, 2017

@MikeMcQuaid Rebased

JCount added some commits Jun 26, 2017

version: improve parsing of url-only, non-filename versions
add support for styles prefixed with a r,v,V and an optional _ ,
and styles with four groups of digits, seperated by periods;
combinations of the two are also supported

@JCount JCount force-pushed the JCount:version-improve-url-only-version-parsing branch from 0bf56a6 to d50312a Jun 30, 2017

@JCount

This comment has been minimized.

Copy link
Contributor

JCount commented Jun 30, 2017

@MikeMcQuaid Rebased again so all the regex changes are in one commit to make the commit history a bit less fragmented.

@JCount JCount merged commit 0a50521 into Homebrew:master Jun 30, 2017

3 checks passed

codecov/patch 100% of diff hit (target 65.78%)
Details
codecov/project 65.78% (+0%) compared to bfe4eed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@JCount JCount deleted the JCount:version-improve-url-only-version-parsing branch Jun 30, 2017

@ilovezfs ilovezfs referenced this pull request Jun 30, 2017

Merged

Remove redundant version specifications #15158

2 of 4 tasks complete

@Homebrew Homebrew locked and limited conversation to collaborators May 4, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.