Skip to content

feat: add new property argsString #139

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, 2020
Merged

feat: add new property argsString #139

merged 7 commits into from
May 8, 2020

Conversation

felipecrs
Copy link
Contributor

Fixes #138

@felipecrs
Copy link
Contributor Author

I'm testing this way:
l0p7925PsW

@rogalmic
Copy link
Owner

rogalmic commented May 7, 2020

Hmm, I like this, since it takes parsing away from extension :)

Did you test the case where argsString is not set in launch.json?

@felipecrs felipecrs changed the title feat: add new property argsString [WIP] feat: add new property argsString May 7, 2020
@felipecrs
Copy link
Contributor Author

I forgot to say, it is WIP. I should at least test it more extensively.

@felipecrs
Copy link
Contributor Author

@felipecrs
Copy link
Contributor Author

Hmm, I like this, since it takes parsing away from extension :)

Did you test the case where argsString is not set in launch.json?

Tested, and now it works. However, the solution is a bit clunky.

JyA71SQXCu
https://github.com/felipecassiors/vscode-bash-debug-test

@felipecrs
Copy link
Contributor Author

@rogalmic what do you think now?

@rogalmic
Copy link
Owner

rogalmic commented May 7, 2020

I would prefer to additionally handle empty argString in extension.ts -> resolveDebugConfiguration(), just to keep handling consistent. The changes in other files are fine.

See how args are handled:

		// Fill non-"required" attributes with default values to prevent bashdb (or other programs) from panic
		if (!config.args) {
			config.args = [];
		}
		else if (!Array.isArray(config.args)) {
			return vscode.window.showErrorMessage("Please specify `args` as array of strings in launch.json.").then(_ => { return undefined; });
		}

@felipecrs
Copy link
Contributor Author

felipecrs commented May 7, 2020

Fixed due to your comment, way better indeed.

I also used a new approach for your argument parsing. shell-quote already provides a method for transforming args array into a string, so why not use it instead? And also, I perform no parsing anymore at argsString. The value should be already properly escaped under JSON, and in case we use the input, it will also write the values accordingly in the JSON. This enabled a much cleaner usage, take a look at how do I pass environment variables and even commands expansion.

QhjeBrTdQC

oiYD8n6lBN

@felipecrs
Copy link
Contributor Author

felipecrs commented May 7, 2020

I think this is now ready for you to review, or test. I suggest merging this PR using squash.

@felipecrs felipecrs changed the title [WIP] feat: add new property argsString feat: add new property argsString May 7, 2020
@rogalmic
Copy link
Owner

rogalmic commented May 8, 2020

OK, looks very good. I will merge this to master, then test a bit. Afterwards it will be released.

It will be available for manual install* under downloads@latest link in:
https://marketplace.visualstudio.com/items?itemName=rogalmic.bash-debug

  • This will have same version as current release, so manual uninstall + vscode restart is needed.

@rogalmic rogalmic merged commit 087cc29 into rogalmic:master May 8, 2020
@felipecrs felipecrs deleted the argsstring branch May 8, 2020 15:58
@felipecrs
Copy link
Contributor Author

Awesome, I'm already using it! 👍

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.

Accept additional parameter argsString for arguments in string format
2 participants