Quarks Windows fixes #1956
Quarks Windows fixes #1956
Conversation
| ^string.findRegexp(regex.isPath).size != 0 | ||
| }, { | ||
| ^(string.findRegexp(regex.isPath).size != 0).and(string.findRegexp(regex.isURL).size == 0) | ||
| }); |
There was a problem hiding this comment.
but how can a URL ever match "\\\\|/" ? are you sure that's what happened with that example you sent me ?
There was a problem hiding this comment.
oh I see, it is a bad regexp:
r = "\\\\|/"
u = "https://github.com/crucialfelix/crucial-library"
u.findRegexp(r)
[ [ 6, / ], [ 7, / ], [ 18, / ], [ 31, / ] ]
I don't know why there is |/ in there
p = "C:\\Users\\Rainer\\Projects\\sc\\supercollider\\build_MW_IPCfix_Rl\\editors\\sc-ide\\Release";
u = "https://github.com/crucialfelix/crucial-library";
r = "\\\\|/";
p.findRegexp(q); // truthy
u.findRegexp(q); // truthy
q = "\\\\"; // simpler: just detect one slash
p.findRegexp(q); // truthy
u.findRegexp(q); // falsey
and then no need for two regexp checks or an if statement there.
There was a problem hiding this comment.
r = "\\\\|/"
First it's not from me, but I think it's correct, though it's likely over-general.
You get 4 "\" because it looks for an already escaped "\". So it looks for "\\" and escapes each of them in the match string
"/" has to be there because that's also allowed in Windows. It would be nice to leave it away, as it would make distinguishing URLs a lot easier, but you wouldn't want to tell people "don't use forward slashes" if it's actually nicer to do so (no escaping needed, cross-platform...). I guess quite a bit of code depends on that "/" can be used as folder delimiter in Windows too.
| @@ -80,7 +80,11 @@ Git { | |||
| // all tags | |||
| // only includes ones that have been fetched from remote | |||
| ^tags ?? { | |||
| raw = this.git(["for-each-ref --format='%(refname)' --sort=taggerdate refs/tags"]); | |||
| if(thisProcess.platform.name !== 'windows', { | |||
| raw = this.git(["for-each-ref --format='%(refname)' --sort=taggerdate refs/tags"]); | |||
There was a problem hiding this comment.
just a note to clarify for others: sh family of shells require quote marks
crucial-library ❯ git for-each-ref --format=%(refname) --sort=taggerdate refs/tags
zsh: missing end of string
@bagong is saying that windows doesn't want the quote, otherwise he gets:
C:\Users\Rainer\AppData\Local\SuperCollider\downloaded-quarks\crucial-library>git for-each-ref --format='%(refname)' --sort=taggerdate refs/tags
…
'refs/tags/for-SC-3.4.1'
'refs/tags/for-sc-3.5rc1'
'refs/tags/4.0.0'
'refs/tags/4.0.1'
'refs/tags/4.1'
'refs/tags/4.1.1'
'refs/tags/docs-syntax-fixes'
'refs/tags/ilisp-help'
'refs/tags/4.1.3'
'refs/tags/4.1.4'
'refs/tags/4.1.5'
|
It's from james harkins. your explanation rings true — thanks. so you are right, we do need to reject URLs. regexp.isUrl could be just "http" and it's fine to call check that on all platforms and thus avoid the extra if statement. but I guess it does work now so maybe we should just move on. its up to you - make minor change to remove the extra if branch or should we just merge it as is ? |
|
Well, I think I've still seen a statement using "git" as protocol, so I thought something more general would be better. I think it's safe to generalize it to |
|
Oh, it's actually correct, ya, one is a symbol and the other is an integer... So ignore that ;) Haha, sometimes copy-pasting others code prevents realizing details ;) |
|
Maybe @jamshark70 wants to lend an eye and commit if he thinks it's okay. |
|
It looks good to me. You're right, I was trying to match paths with either a backslash or forward slash. But, if I had a nickel for every time I messed up the escapes in a regex, I could pay Donald Trump to drop out of the election. I'm ok with any code that works -- you're more than welcome to fix my mistakes ;) |
|
Your original fix was right, it's still right. We might comment it with an explanation so it's not so "wtf?" for future generations. |
|
Yep, the problem is not that "/" is in there but that the program logic requires URLs to be excluded. Thanks for looking and sharing responsibility by committing! |
(includes code form crucialfelix' pr #1954 )