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

Allow shim's dirname in path and args #1

Closed
wants to merge 4 commits into from
Closed

Allow shim's dirname in path and args #1

wants to merge 4 commits into from

Conversation

felipecrs
Copy link

@felipecrs felipecrs commented Apr 10, 2021

No description provided.

@felipecrs
Copy link
Author

I'm trying to fix 71#10, but I have no clue on what I'm doing, I'd really appreciate a little help. :(

@felipecrs
Copy link
Author

The current solution is actually working:

cat C:\Users\felip\AppData\Local\Volta\tools\image\npm\7.9.0\bin\npm.shim
path = C:\Users\felip\AppData\Local\Volta\tools\image\node\14.16.1\node.EXE
args = %~dp0\npm-cli.jsC:\Users\felip\AppData\Local\Volta\tools\image\npm\7.9.0\bin\npm.exe --version
7.9.0

Any tips for improving the performance?

@God-damnit-all
Copy link

RegEx isn't really something you use when you're trying to get the fastest performance possible.

I don't know anything about C++, but maybe @kiennq could vouch for using a function like this? https://www.cplusplus.com/reference/string/string/replace/ It's just a simple replace operation, no RegEx required.

@kiennq
Copy link
Owner

kiennq commented Jan 26, 2022

Sorry for the late reply, and yes, this feature looks useful. It would be great if you can make it ready for merge :)

shim.cpp Outdated Show resolved Hide resolved
shim.cpp Outdated Show resolved Hide resolved
shim.cpp Outdated Show resolved Hide resolved
@@ -105,7 +114,9 @@ std::tuple<std::wstring_p, std::wstring_p> GetShimInfo()
continue;
}
}

std::wstring key = std::wstring(L"%~dp0");
path.emplace(std::regex_replace(path.value(), std::wregex(key), dirname));
Copy link
Owner

Choose a reason for hiding this comment

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

Please use replace instead of regex version, which seems to be overkilled

Choose a reason for hiding this comment

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

@felipecrs I was going to compile and test out the PR on your behalf, but I noticed you didn't implement this suggestion. Please do, and remember to remove import <regex> from the top as well.

Copy link
Author

@felipecrs felipecrs Feb 8, 2022

Choose a reason for hiding this comment

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

@ImportTaste I tried, but I could not think of way to do. If you know how to do it, I would appreciate if you could do it.

Copy link
Author

Choose a reason for hiding this comment

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

I invited you to my fork, so that you can change push to the PR if you need.

shim.cpp Show resolved Hide resolved
shim.cpp Show resolved Hide resolved
felipecrs and others added 3 commits January 29, 2022 10:39
Co-authored-by: kiennq <kien.n.quang@gmail.com>
Co-authored-by: kiennq <kien.n.quang@gmail.com>
Co-authored-by: kiennq <kien.n.quang@gmail.com>
@felipecrs
Copy link
Author

felipecrs commented Jan 29, 2022

@kiennq, first, thanks for the review. You did everything!

Being completely honest with you, I blindly applied all your suggestions, but I have no clue whether they work or not. I have almost zero knowledge about C/C++, and I'm struggling a lot to even being able to build it.

Would you mind taking the PR by yourself? I have given permission to you to edit it. I would be so grateful.

My goal with the PR was to just highlight the use-case as a PoC, and I'm happy it served that purpose.

@God-damnit-all
Copy link

@kiennq I would also like to voice support for integrating the code, I hit a situation today where relative pathing would really be useful.

@MetalSpork
Copy link

Is this still being worked on?

@felipecrs
Copy link
Author

Not from my side. :)

@kiennq kiennq force-pushed the main branch 3 times, most recently from de2c0ec to e0fb559 Compare March 5, 2024 12:48
kiennq added a commit that referenced this pull request Mar 6, 2024
Co-authored-by: felipecrs
kiennq added a commit that referenced this pull request Mar 6, 2024
Co-Authored-By: felipecrs <felipecassiors@gmail.com>
@kiennq kiennq closed this Mar 6, 2024
@felipecrs
Copy link
Author

Thanks a ton, @kiennq!

@God-damnit-all
Copy link

@kiennq This does not appear to be working for me. I've tried:

path = C:\my\path\to\Sublime Text\subl.exe
args = "%~dp0\test file.txt"

and

path = cmd.exe
args = /K ECHO/%~dp0

In the case of the former, Sublime Text opens the path as if it's a new unsaved file:

image

In the case of the latter, it just echoes %~dp0

image

I thought maybe that perhaps it might only work if the args started with it, but that didn't work either:

path = C:\my\path\to\Sublime Text\subl.exe
args = %~dp0\test.txt

image

@kiennq
Copy link
Owner

kiennq commented Mar 6, 2024

@ImportTaste It works for me though.
image

Are you using the latest pre-built one?
It doesn't contain this change yet. Let me kick off a new build.

@God-damnit-all
Copy link

@ImportTaste It works for me though. image

Are you using the latest pre-built one? It doesn't contain this change yet. Let me kick off a new build.

Thank you, apparently there was an issue with my script because the default branch was renamed from master to main.

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.

4 participants