fix: assets ending in ".$OS.$ARCH" #71
Conversation
|
Hi, thanks for your PR! I'm pretty finicky about my projects (see this blog post for details), so I rarely merge a PR as-is. I can move forward on your PR in one of two ways:
Please let me know which approach you'd prefer. If I don't hear from you before I get around to working on this PR I'll go with option #1. Thanks again for your contribution! |
ubi/src/os.rs
Outdated
| pub(crate) static PLATFORM_RE: Lazy<Regex> = Lazy::new(|| { | ||
| Regex::new( | ||
| &[all_oses_re(), all_arches_re()] | ||
| .iter() | ||
| .map(|r| format!("({})", r.as_str())) | ||
| .join("|"), | ||
| ) | ||
| .unwrap() | ||
| }); |
There was a problem hiding this comment.
I think this can be constructed in the extension module by importing the relevant fns from os and arch.
There was a problem hiding this comment.
idk how significant it is but won't that involve recompiling the final regex many times?
There was a problem hiding this comment.
I thought the use of Lazy would mean it's only compiled once, right?
There was a problem hiding this comment.
Or rather, can't Lazy also be used in the extension module? Maybe there's some issue with doing that?
There was a problem hiding this comment.
all_oses_re is not wrapped in Lazy and gets called many times, my PR fixes that though
There was a problem hiding this comment.
Ah, right. I just pushed a change to make both of these static Lazy<Regex> vars instead of functions.
jdx
force-pushed
the
sops
branch
3 times, most recently
from
fc37558
to
ad27390
Compare
|
rebased |
|
Is it okay if I push to this PR. I want to tweak the commit message before merging. |
|
yeah that's fine |
…ecture, like ".linux.amd64" Fixes jdx/mise#2854
Fixes jdx/mise#2854