-
Notifications
You must be signed in to change notification settings - Fork 22
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
RunRenpyViaplugin #291
RunRenpyViaplugin #291
Conversation
Hi there, thank your interest in the project and taking the time to build this PR! :) This is definitely a good start and the code looks good. (I will need to take some time later to test it before I approve the code). That said, it would be nice if you can link this command to the 'play/run' button. That way we use the default behavior, instead of requiring users to run a command. |
I just looked through the documentation for this. I think the only way to do this, is through the debugger API. https://code.visualstudio.com/api/extension-guides/debugger-extension In our case, the full debugger is a bit of a stretch. But I think we should be able to use the mock debugger example they provided, then strip out all the debugger options and only provide the launch/run functionality. Let me know if that's something you'd be willing to help with, or if you'd rather leave it as a command for now. Given that this is a bit more work, I totally understand if you do. |
I think we might be able to add a launch configuration without the debugger. That would be best. In our case the 'type' should then be Also see this: |
I'm happy to try out doing this. Ill read through the vscode extension docs you sent and send an update |
Thank you very much for your contribution. I look forward to testing and reviewing it soon. |
I noticed that your formatting is configured to 2 spaces instead of four. Did you use prettier? The .editorconfig does use 4 spaces for ts files. Let me know if that setting doesn't work anymore |
Ill try rerunning it, I should have prettier installed on the machine tho gimme a sec |
Done now. Think my prettier for some reason didn't format the workspace on save like I set it to |
I'll try to reformat again, should be able to whitespace diff instead of full diff instead, but hopefully this works. |
Yes perfect, that worked. I'll review it in a bit |
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.
Awesome work! Looking really good, some minor things and then we can get this merged! 😄
src/extension.ts
Outdated
if (!config) { | ||
window.showErrorMessage("Ren'Py executable location not configured or is invalid."); | ||
} else { | ||
if (isValidExecutable(config.renpyExecutableLocation)) { |
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.
You can move this check to the line above with an || or comparison. (So 'if not config or renpy executable location not set'.) That way you don't need the copy the message log code :)
(Also in case you didn't know, that works because if statements do early out. Meaning the config is checked first, and only if true will it check the remaining parts)
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.
God it, fixed this now
src/extension.ts
Outdated
window.showErrorMessage("Ren'Py executable location not configured or is invalid."); | ||
} else { | ||
if (isValidExecutable(config.renpyExecutableLocation)) { | ||
//this is kinda a hob botched together attempt that I'm like 30% certain has a chance of working |
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.
Did you test if it works? If it does you should remove the 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.
Did not manage to test if it works yet, will try after this commit
src/extension.ts
Outdated
window.showInformationMessage("Ren'Py is running successfully"); | ||
} | ||
} else { | ||
window.showErrorMessage("Ren'Py executable location not configured or is invalid."); |
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.
Remember to remove this one if you addressed my first comment :)
src/extension.ts
Outdated
function RunWorkspaceFolder(): boolean { | ||
const config = workspace.getConfiguration("renpy"); | ||
const renpy = config.renpyExecutableLocation; | ||
if (isValidExecutable(renpy)) { |
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.
Remember that the config can be null, you should use the same check as above.
(Even if you know you checked, if someone in the future wants to use this function, it should be safe to use on it's own)
src/extension.ts
Outdated
if (isValidExecutable(renpy)) { | ||
const renpyPath = cleanUpPath(Uri.file(renpy).path); | ||
const cwd = renpyPath.substring(0, renpyPath.lastIndexOf("/")); | ||
let wf = getWorkspaceFolder(); |
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.
I see now you copied it from the function below, so no worries. But my feedback should still be applied.
I think this can be const. Also please use full (short) descriptive names for variables. If I want to know what wf
means, I now have to look at the definition.
I can't seem to run the extension tests on my machine? Or at least the secondary checker one you have on there |
One last thing :p Sorry, what extension tests are you talking about? |
Also yeah, I did mean running the extension, I'm installing node.js now lmao, didn't even realize it was needed. |
Your last commit converted everything to two spaces again 😁 |
Yes looking good. LGTM! 🎉🎉 |
* Edited extensions method to add way to run renpy * Added the command to the pallete via json * Adding debug config to allow renpy to be played from play button * changed type to cmd * reformatted with prettier * This format might work? * Refactored if statement to make it more readable * Refactoring of folder names and chaning if statements * This commit fixed the issues in comment https://github.com/LuqueDaniel/vscode-language-renpy/pull/291#discussion_r1170180867, , refactoring if statement * reformated through magical means... God I hate switching computers * Reformated with the correct file, please ignore previous one
* Edited extensions method to add way to run renpy * Added the command to the pallete via json * Adding debug config to allow renpy to be played from play button * changed type to cmd * reformatted with prettier * This format might work? * Refactored if statement to make it more readable * Refactoring of folder names and chaning if statements * This commit fixed the issues in comment https://github.com/LuqueDaniel/vscode-language-renpy/pull/291#discussion_r1170180867, , refactoring if statement * reformated through magical means... God I hate switching computers * Reformated with the correct file, please ignore previous one
* Edited extensions method to add way to run renpy * Added the command to the pallete via json * Adding debug config to allow renpy to be played from play button * changed type to cmd * reformatted with prettier * This format might work? * Refactored if statement to make it more readable * Refactoring of folder names and chaning if statements * This commit fixed the issues in comment https://github.com/LuqueDaniel/vscode-language-renpy/pull/291#discussion_r1170180867, , refactoring if statement * reformated through magical means... God I hate switching computers * Reformated with the correct file, please ignore previous one
* Edited extensions method to add way to run renpy * Added the command to the pallete via json * Adding debug config to allow renpy to be played from play button * changed type to cmd * reformatted with prettier * This format might work? * Refactored if statement to make it more readable * Refactoring of folder names and chaning if statements * This commit fixed the issues in comment https://github.com/LuqueDaniel/vscode-language-renpy/pull/291#discussion_r1170180867, , refactoring if statement * reformated through magical means... God I hate switching computers * Reformated with the correct file, please ignore previous one
* Chore: Allow 000*.rpy files in renpy/common (#300) * RunRenpyViaplugin (#291) * Edited extensions method to add way to run renpy * Added the command to the pallete via json * Adding debug config to allow renpy to be played from play button * changed type to cmd * reformatted with prettier * This format might work? * Refactored if statement to make it more readable * Refactoring of folder names and chaning if statements * This commit fixed the issues in comment https://github.com/LuqueDaniel/vscode-language-renpy/pull/291#discussion_r1170180867, , refactoring if statement * reformated through magical means... God I hate switching computers * Reformated with the correct file, please ignore previous one * Ignore dist folder (#303) * Ignore dist folder (#301) * Chore(Enhancement) - Add a command that cleans up compiled files. * Update the command title to match the convention. * Use await/async syntax * Merge pull request #304 from a2937/old-file-command Chore(Enhancement) - Add a command that cleans up compiled files. --------- Co-authored-by: a2937 <[email protected]> Co-authored-by: seanj29 <[email protected]> Co-authored-by: Daniel Luque <[email protected]>
* Chore: Allow 000*.rpy files in renpy/common (#300) * RunRenpyViaplugin (#291) * Edited extensions method to add way to run renpy * Added the command to the pallete via json * Adding debug config to allow renpy to be played from play button * changed type to cmd * reformatted with prettier * This format might work? * Refactored if statement to make it more readable * Refactoring of folder names and chaning if statements * This commit fixed the issues in comment https://github.com/LuqueDaniel/vscode-language-renpy/pull/291#discussion_r1170180867, , refactoring if statement * reformated through magical means... God I hate switching computers * Reformated with the correct file, please ignore previous one * Ignore dist folder (#303) * Ignore dist folder (#301) * Chore(Enhancement) - Add a command that cleans up compiled files. * Update the command title to match the convention. * Use await/async syntax * Adds PR ci to master Fixes #306 * Fixes codeql-analysis.yml format * Adds setup action * Fix package lock after merge --------- Co-authored-by: a2937 <[email protected]> Co-authored-by: seanj29 <[email protected]> Co-authored-by: Daniel Luque <[email protected]>
Hey, I'm trying to use the command
I can see that it should be set in How am I supposed to set it? |
Hi there, I think there might be no UI entry for this (going off your comment). In that case you can only set it manually by adding this entry in the setting json config |
So, the parameter is not autocompleted and not recognised (it appears half transparent and hovering it shows tooltip "Unknown Configuration Setting") but surprisingly setting it to the full path of renpy.sh (on Linux) actually replaces the "Ren'Py executable location not configured or is invalid." error bottom-right popup with a centered popup window: Clicking on "Install cmd Extension" will suggest a few bash/command-related extension to install, but it doesn't seem quite right, so I'm not sure what to do now. For now I just run Renpy via a custom command using F5Anything... EDIT: since this PR is merged I'll copy this post to #115 to continue discussion. |
I've got some time, so I'm gonna try and remake this in better way, and try to get running working from GUI too. |
realised that its alredy been fixed in #115 oops |
I don't have any expereince with typescript, so it may be a bit of a botched job, and I can't seem to get node.js to run on my machine, but this should be able to deal with https://github.com/LuqueDaniel/vscode-language-renpy/issues/115#issue-1239736338. I used a child process to run terminal command similar to what is done to watch for renpy compile data, but it should spawn a new renpy process in a new window.