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

Post Template Task with Arguments #372

Draft
wants to merge 71 commits into
base: master
Choose a base branch
from

Conversation

theschitz
Copy link
Collaborator

@theschitz theschitz commented Apr 17, 2022

Related to #324.

Changes proposed in this pull request:

  • Adds the possibility to pass arguments for commands defined in tasks.

Additional comments

Ehm... don't let the commit history fool you. Branched out of the dev branch for #324.

@theschitz theschitz added the enhancement New feature or request label Apr 17, 2022
@theschitz theschitz requested a review from jwikman April 17, 2022 18:48
@theschitz
Copy link
Collaborator Author

Hmm, I wonder... have I broken the test suites again 😠

@theschitz theschitz marked this pull request as draft April 17, 2022 20:59
Copy link
Owner

@jwikman jwikman left a comment

Choose a reason for hiding this comment

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

Don't know why the tests are failing, but I guess you figure out tomorrow morning or so.. 😜

Just had one reflection while looking at this

export interface TaskRunnerItem {
description: string;
command: string;
arguments?: CommandParameter[];
Copy link
Owner

Choose a reason for hiding this comment

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

I think we have to go for any[] here...
image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

unknown[] maybe?

@jwikman
Copy link
Owner

jwikman commented Apr 23, 2022

I think we have quite a challenge here... (not only with your test...)

I don't know if we could support arguments of other than simple data types, and (meaningful) commands with only simple data types seems to be a bit rare (at least the built-in commands)...

Maybe we need to let the user provide a datatype (from a list of supported ones), and we cast them before sending them to the executeCommand function...?

@theschitz
Copy link
Collaborator Author

I think we have quite a challenge here... (not only with your test...)

I don't know if we could support arguments of other than simple data types, and (meaningful) commands with only simple data types seems to be a bit rare (at least the built-in commands)...

Maybe we need to let the user provide a datatype (from a list of supported ones), and we cast them before sending them to the executeCommand function...?

This could get interesting... I guess we would need to define the structure of the supported data types in the syntax.json as well? Otherwise we would cast from an object with unknown properties.

@jwikman
Copy link
Owner

jwikman commented Apr 25, 2022

This could get interesting... I guess we would need to define the structure of the supported data types in the syntax.json as well? Otherwise we would cast from an object with unknown properties.

Exactly... The most obvious one to support is Uri (see https://code.visualstudio.com/api/references/commands for built-in commands)
But other than that, I don't know... Do we really want to go down this road?
Maybe we should settle with simple datatypes, plus URI? The simple datatypes is easily supported (pretty much what you support right now), but how would the syntax look like for an URI string?
It should support

  • http URL - URI.parse(path)
  • file:// syntax - URI.parse(path)
  • file path syntax - URI.file(path)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants