Skip to content

Add optional UI to prompt for script args. #707

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

Merged
merged 7 commits into from
May 8, 2017

Conversation

rkeithhill
Copy link
Contributor

Address one issue #705

@rkeithhill
Copy link
Contributor Author

rkeithhill commented May 2, 2017

This is not quite ready yet. I'm waiting on a resolution or workaround to this issue - microsoft/vscode#25712

rkeithhill added 2 commits May 2, 2017 09:01
This currently has an issue where you can't clear the previous args to send no args.  This is has been addressed in VSCode for May 2017 drop - microsoft/vscode#25712 (comment)
@daviwil
Copy link
Contributor

daviwil commented May 4, 2017

Awesome! I suppose we'll wait for the next VS Code update before shipping this in the PS extension?

@rkeithhill
Copy link
Contributor Author

Yes. I keep testing daily builds of VSCode Insiders but haven't observed the fixed behavior yet.

@rkeithhill
Copy link
Contributor Author

OK, the issue is fixed in today's insiders build. It should ship in the (late) May release.

This is for the prompt for script args UI.  If on >= 1.13 then save state. If less than that do not save state because there is a bug in VSCode that will make it impossible to specify no args after specify some args -even across different sessions.
@rkeithhill
Copy link
Contributor Author

One more thing to do. Need to figure out a debug config snippet for this. I'll propose one here in a bit.

Also reworded the dbg configuration names.  The drop down control in VSCode is on the narrow side.  By putting in PowerShell, we take up most of the width. I notice that Node doesn't bother to prefix their dbg configs "Launch Program", "Mocha Tests".  However I've already used cross language dbg configs in a single project and it is nice to know which language the dbg config is for.  Therefore I think a good compromise is to prefix our dbg snippets with "PS".  In the launch.json file, you can still see the config type = "PowerShell".

I'll post some screen shots so you can see what this results look like.
@rkeithhill
Copy link
Contributor Author

rkeithhill commented May 7, 2017

I reworded the dbg configuration names. The drop down control in VSCode is on the narrow side. By putting in PowerShell, we take up most of the width. I notice that Node doesn't bother to prefix their dbg configs e.g.: "Launch Program", "Mocha Tests".

However, I've already used cross language dbg configs in a single project and it is nice to know which language a dbg config is for. Therefore I think a good compromise is to prefix our dbg snippets with "PS". In the launch.json file, you can still see the config type = "PowerShell".

Here is what the Debug configuration list looks like now (from the Examples folder):
image

This is what the Add Configuration choices look like:
image

@rkeithhill
Copy link
Contributor Author

I suppose we'll wait for the next VS Code update before shipping this in the PS extension?

We can ship this before the May VSCode release drops. I added code to check the VSCode version and I only enable the "save the prev args" features if we're on 1.13 or later.

@rkeithhill
Copy link
Contributor Author

I'm not sure I like the args being saved across separate VSCode sessions. Let me see if I can find a way to store the args just for the current session.

The args are remembered across VSCode invocations but at least each workspace has its own args remembered.  That actually might be a feature.
@daviwil
Copy link
Contributor

daviwil commented May 8, 2017

Using "PS" as the prefix just feels wrong to me even though I can understand your reason for doing it. As far as I know, nobody has complained about not being able to see the name in the debug UI. Maybe we should leave it the way it is so that it's clearer?

I suppose there's also the possibility of rewording them slightly. Instead of "PowerShell Launch Current File" it could be "Launch Current PowerShell File", "Interactive PowerShell Session" instead of "PowerShell Interactive Session", etc. What do you think?

@rkeithhill
Copy link
Contributor Author

rkeithhill commented May 8, 2017

OK putting PS back to PowerShell. I think it is better to leave PowerShell first because so that in the mixed debug configuration scenario, the PowerShell configs sort next to one another. PR update coming.

@daviwil
Copy link
Contributor

daviwil commented May 8, 2017

Thanks Keith! This change is pretty awesome, I like the new feature a lot. Is it ready to be merged now?

@rkeithhill
Copy link
Contributor Author

I think so. Do you think it is OK to remember scripts args between sessions using workspaceState?

@daviwil
Copy link
Contributor

daviwil commented May 8, 2017

Yeah, I think that's a great way to do it. We might need a way to clear the stored args in the future, but I wouldn't worry about it until someone asks for it. Merging it now, thanks man!

@daviwil daviwil merged commit 2b97f5a into develop May 8, 2017
@daviwil daviwil deleted the rkeithhill/script-args-ui branch May 8, 2017 17:54
@rkeithhill
Copy link
Contributor Author

I think this will make for a nice little debugging enhancement.

@daviwil daviwil modified the milestones: 1.0.0-beta1, 1.0.0 May 9, 2017
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.

3 participants