-
Notifications
You must be signed in to change notification settings - Fork 2k
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
lib to check init.bat's custom args #1758
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be handled differently. init.bat
already accepts named args that could break your intended use case. Maybe an else
in the init.bat
command parser code that adds all unrecognized arguments to a CMDER_FLAGS
environment variable:
else (
set "CMDER_FLAGS=%CMDER_FLAGS% %1"
}
That can be passed to `user-profile.cmd:
set "initialConfig=%CMDER_ROOT%\config\user-profile.cmd"
if exist "%CMDER_ROOT%\config\user-profile.cmd" (
REM Create this file and place your own command in there
call "%CMDER_ROOT%\config\user-profile.cmd" %CMDER_FLAGS%
)
if defined CMDER_USER_CONFIG (
set "initialConfig=%CMDER_USER_CONFIG%\user-profile.cmd"
if exist "%CMDER_USER_CONFIG%\user-profile.cmd" (
REM Create this file and place your own command in there
call "%CMDER_USER_CONFIG%\user-profile.cmd" %CMDER_FLAGS%
)
)
user-profile.cmd
could then do something like:
echo %* | find -i "/noautorun"
if "%ERRORLEVEL%" == "1" (
echo Auto run
call devDocs
call SublimeText
call vsCode
)
If OK with other @cmderdev/trusted-contributors .
@daxgames Sorry that I've missed By the way, it seems that v1.3.5 isn't match current branch at all. Is that OK? |
@xiazeyu - Yes 1.3.5 is the released version of cmder. The master branch has unreleased changes. Also I made a couple more small changes just to make things a little clearer. @cmderdev/trusted-contributors do you approve of this change? |
vendor/init.bat
Outdated
echo :: If found... | ||
echo :: echo %* | find /i "/noautorun">nul | ||
echo :: if "%ERRORLEVEL%" == "0" ( | ||
echo :: call vsCode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is vsCode
a batch file, or a subroutine? Why is it called here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not it's a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DRSDavidSoft
vsCode
itself is an alias which I used to run vsCode.
I put here just to show an realistic example, it can be replaced by anything.
It is possible that the user may feel confused with what it is, like you did, but I does'nt have a clear idea of what to write here yet.
@daxgames @DRSDavidSoft I've optimize the comments about this feature, have a look plz. |
@daxgames ready to merge or nah? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
• This functionality should be documented in README.md
, please add a commit which includes your description. @xiazeyu
• I think the argument handling should be done using one of the recently added libraries. lib_base.bat
, perhaps? @daxgames
• Also, I think plugins should be called from the config
directory, maybe user-plugins.bat
? If possible, init.bat
should call that file with the appropriate library already parsed. My reasoning is that init.bat
gets overwritten on each update, thus not a reliable location for plugins.
P.S. 1@daxgames I can write a
P.S. 2@xiazeyu On another note, we have a Wiki page explaining on how to integrate Cmder with VS Code, here: https://github.com/cmderdev/cmder/wiki/Seamless-VS-Code-Integration If you have something useful in your Here's also the wiki page for Sublime 3 integration: https://github.com/cmderdev/cmder/wiki/Seamless-Sublime-Text-3-Integration |
@DRSDavidSoft I don't understand the argument handling question and I don't understand the plugins comment because there is no argument handling because any unknown argument to is just passed to user-profile.cmd and there are no plugins so can you explain? |
@daxgames Sure. I propose adding a simple wrapper to be used as argument handler, instead of
Of course, this is just an idea. If this doesn't make any sense, let's ommit it and stick to parsing arguments directly in My other main point (regarding documentation) is that this functionality (and how to use it with |
I think document this function in In my I think your wapper idea is excellent, that will make I also think it will be too much complex if I will try if I can put this function into something like |
@xiazeyu Glad to hear that, so I'll wait to see what you'll commit for the mentioned subroutines. If you need help, you can count on me. I think this functionality will be great in Cmder! :) |
@DRSDavidSoft @daxgames @Stanzilla I've done my work, and have a look if this have something unexpected. I recommend to update the Wiki page when the PR is merged. |
@xiazeyu Cmder has a concept of importable libraries in We put the libraries in I am not sure I like Thoughts? |
Please use %flag_exists% instead of using have
@daxgames See the latest commit. I'm poor at naming functions, so I really think flag_exists is a better idea. |
@daxgames, thoughts? @xiazeyu I'm reviewing the PR, if everything's alright I'll ask @Stanzilla to merge it. Thanks for the contribution, btw! |
@DRSDavidSoft @daxgames @Stanzilla |
We just released 1.3.6. It has a couple of issues that have already been fixed. I think we might need to do another one sooner rather than later. |
These concerns are just about some style issue, and since it can work, I don't think it matters a lot. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I have these issues in mind:
1- Should Cmder.exe
pass the flag to init.bat
?
2- The code for flag_exists.cmd
library needs a bit of refactoring
3- The readme notes require more intuitive explanation and better formatting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some more questions
@xiazeyu Please have a look at my review; once you reply them I can make a PR to your branch to address them. Some additional points:
|
I passed to Thanks for your idea and review anyway. |
@xiazeyu Thanks for the quick replies. I'll commit my suggested edits and then make a PR into your branch before this one is merged. Might I suggest making this new flag feature work when called from both Also, @daxgames, your thoughts please. |
I'm totally agree with you. |
@DRSDavidSoft and @xiazeyu I am not sure we need to add this functionality to I have said before I am concerned with adding so much functionality to the launcher that it becomes difficult to maintain. I am also concerned about breaking existing functionality. Currently
or
We can't just throw a random arg at it because it errors on unknown/invalid command line args. If this functionality is added it would need to be in a way that used a prefix like To me the meat of this PR is the library that makes it easy for easy determination of whether a flag was passed or not. Currently the lib does not appear to adhere to the format of our other libraries in I need to look closer though and do a full test and review of the PR, if I am misunderstanding something please clarify. |
@daxgames I think your understanding is exactly the same with me. About pass args to init.bat or launcher:
#1758 (comment) About format: I think some of the difference exists because scripts in If you stick to it, you need to unite the format by yourself(sorry about that), since I can't get the rule of format(stylistic issues, comments style, best process flow as a lib).(It's think this solving this issue is not like using ESLint, which has a fixed rules to follow). Thanks for your review and test anyway. |
I have adapted it to a cmder library and can PR back to @xiazeyu. |
I changed my mind I did not make it a library. In my opinion In my PR to @xiazeyu I have:
|
Regarding the launcher argumentsTo be clear, I didn't plan to throwing random arguments from the launcher to On the note that is it necessary or not, I would imagine as the launcher, it should be possible to pass any Regarding the formatI had planned on refactoring @xiazeyu's batch file to match the other libs (like you initially did), but considering the format and the nature of the file, I also agree with separating it from the other libs. May I ask why Regarding maintainabilityI made a note at the end of #1873 (comment) I noticed that @daxgames has already made some of the necessary changes in his PR, here: https://github.com/xiazeyu/cmder/pull/1. |
I see found what cexec was. I just think it is much more accurate than flag_exists. I would expect flag_exists to simply return a binary yes or no but it does much more than that. The only way I can see the launcher passing flags to init.bat is trough an environment variable like %CMDER_ARGS%. The variable could then be added to the default cmder tasks in conemu. |
The only issue with the above approach is it is a manual change to the cmder task config for existing users that already have a Just spit balling here but we could instead add some code to |
Does it mean that you can use If |
flag_exists.cmd to cexec
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 👍
This commit deals with the issue when I tried to use
init.bat
for bothcmder.exe
andCode.exe
(VS Code).in the user-profile, I set an Autorun function:
In
cmder.exe
, these applications will be run once I startcmder.exe
,and will not run when I use it in
VSCode
to achieve this, I passed the arguments received by
init.bat
touser-profile.cmd
and it seems OK.