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 #33 ? #34

Merged
merged 2 commits into from
Feb 4, 2023
Merged

Fix #33 ? #34

merged 2 commits into from
Feb 4, 2023

Conversation

jpambrun
Copy link
Contributor

@jpambrun jpambrun commented Feb 3, 2023

This could still be much better.

This could still be much better.
@jpambrun
Copy link
Contributor Author

jpambrun commented Feb 3, 2023

This is untested.

@jpillora
Copy link
Owner

jpillora commented Feb 3, 2023

Ah you have a PR - thank you :) will check it out soon

@@ -6,7 +6,7 @@ import (
)

var (
archRe = regexp.MustCompile(`(arm64|arm|386|amd64|32|64)`)
archRe = regexp.MustCompile(`(arm64|arm|386|amd64|32|64|x86_64|aarch64)`)
Copy link
Owner

Choose a reason for hiding this comment

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

can you plz move these to the front? otherwise it'll return "64"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did try it here[1] beforehand it seemed to macht the longest, but I can change regardless.

[1] https://regexr.com/77j6n

Copy link
Owner

Choose a reason for hiding this comment

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

Oh interesting, TIL
will test in Go soon and confirm

@jpillora
Copy link
Owner

jpillora commented Feb 3, 2023

Just one minor change, otherwise LGTM

@jpillora jpillora merged commit b9eb77b into jpillora:master Feb 4, 2023
@jpillora
Copy link
Owner

jpillora commented Feb 4, 2023

Merged, should go live soon via CI

thanks!

(heading off to sleep 🤞)

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.

2 participants