Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

add paths option to vti diagnostics #2825

Merged
merged 1 commit into from
Jun 1, 2021
Merged

add paths option to vti diagnostics #2825

merged 1 commit into from
Jun 1, 2021

Conversation

gregoirechauvet
Copy link
Contributor

Resolves #2455

vti diagnostics --files src/App.vue src/Form.vue to run VTI only on those two files

$ vti diagnostics --help
Usage: vti diagnostics [options] [workspace]

Print all diagnostics

Options:
  -l, --log-level <logLevel>  Log level to print (choices: "ERROR", "WARN", "INFO", "HINT", default: "WARN")
  -f, --files <files...>      Files to diagnose (default: null)
  -h, --help                  display help for command

@gregoirechauvet gregoirechauvet changed the title VTI add --files option add --files option to vti diagnostics Mar 26, 2021
@rchl
Copy link
Collaborator

rchl commented Mar 26, 2021

Maybe clarify how to pass multiple files? Separate paths with some delimiter or specify multiple --files. If the latter then "files" sounds kinda wrong from the user's perspective because it's plural but accepts only one file?

@gregoirechauvet
Copy link
Contributor Author

gregoirechauvet commented Mar 26, 2021

because it's plural but accepts only one file?

Hum... no. It accepts an array of files. So you can do --files a.vue b.vue c.vue. Isn't it clear enough? Is there a way to make it clearer in the --help dialog?

Or you prefer another API maybe?

@rchl
Copy link
Collaborator

rchl commented Mar 26, 2021

Hum... no. It accepts an array of files. So you can do --files a.vue b.vue c.vue.

Are you sure that even works? Typically in CLIs only one value is allowed for each option. So you would have to quote the value if you would want to include spaces in the value.

So I think that's not clear as it's against my expectations how CLIs work, at least.

Or you prefer another API maybe?

I'll leave the final decision to the project owners.

@rchl
Copy link
Collaborator

rchl commented Mar 26, 2021

And since an optional workspace path is allowed at the end of the CLI, how would the parser know where one of the --files ends and workspace starts?

@gregoirechauvet
Copy link
Contributor Author

Yep, it works, it was running fine locally. For the workspace I was putting it before the --files option, like this:

vti diagnostics src/ --files a.vue b.vue c.vue

It's a valid behavior of commander: https://github.com/tj/commander.js#variadic-option

But you're right. I'm not sure of what to do, I need guidance here. Let me know how the API should look like 🙂

@gregoirechauvet
Copy link
Contributor Author

And since an optional workspace path is allowed at the end of the CLI, how would the parser know where one of the --files ends and workspace starts?

From commander's documentation, you can also use the special argument -- to stop option processing:

vti diagnostics --files a.vue b.vue c.vue -- src/

@rchl
Copy link
Collaborator

rchl commented Mar 29, 2021

Ultimately it's up for module owners to decide but if it's supported by the CLI library used then I guess it's fine.

But I think the new option should be at least documented in https://github.com/vuejs/vetur/blob/master/docs/guide/vti.md, if not in the CLI itself.

@rchl
Copy link
Collaborator

rchl commented Mar 29, 2021

Also, it's maybe not clear what's the interaction when both the --files and workspace are specified.
And it's kinda weird that those are specified separately like that. Would it not be possible to just be able to specify file instead of the workspace argument, without have a separate --files argument?

I mean:

vti diagnostics -- a.vue b.vue c.vue

and then maybe one could also specify both directories and files together:

vti diagnostics -- src/ a.vue b.vue c.vue

@gregoirechauvet
Copy link
Contributor Author

gregoirechauvet commented Mar 29, 2021

vti diagnostics -- a.vue b.vue c.vue

With this syntax, how to distinguish the workspace from the first file? With the .vue extension maybe?

vti diagnostics -- src/ a.vue b.vue c.vue

And with this one, it might suggest that you can more than one workspace, no? Which isn't supported in VTI. Or that you can mix files and folders?

If this is the preferred way, I'm fine with it. Should I update the PR? Or should I wait for the project owner's feedback as you suggested once?

@rchl
Copy link
Collaborator

rchl commented Mar 29, 2021

Maybe don't listen to me but wait for owners of the code.

@yoyo930021
Copy link
Member

yoyo930021 commented Apr 7, 2021

I think it would be nice to be consistent with the ESLint CLI.
https://eslint.org/docs/user-guide/command-line-interface#options

Thank for your PR. 👍

@gregoirechauvet gregoirechauvet changed the title add --files option to vti diagnostics add paths option to vti diagnostics Apr 15, 2021
@gregoirechauvet
Copy link
Contributor Author

gregoirechauvet commented Apr 15, 2021

I've updated the PR to try to match ESLint syntax, as suggested. I haven't implemented the glob syntax, though.

$ vti diagnostics --help
Usage: vti diagnostics [options] [workspace] [paths...]

Print all diagnostics

Options:
  -l, --log-level <logLevel>  Log level to print (choices: "ERROR", "WARN", "INFO", "HINT", default: "WARN")
  -h, --help                  display help for command

From my understanding, VTI can't work without a workspace, right? Because it needs to find the package.json and such things.

Now after the workspace, you can add a list of files and/or directories. Their paths are relative to the workspace.
Then if you want to limit the diagnostics to a file, you also have to pass the workspace:

> vti diagnostics                                 // will run on the current directory as usual, the workspace is still optional
> vti diagnostics . src/App.vue                   // will run only on src/App.vue
> vti diagnostics . src/App.vue src/components/   // will run on src/App.vue and all .vue files inside src/components/
> vti diagnostics src/App.vue                     // will understand src/App.vue as the workspace, and fail

What do you think?

@yoyo930021
Copy link
Member

I've updated the PR to try to match ESLint syntax, as suggested. I haven't implemented the glob syntax, though.

$ vti diagnostics --help
Usage: vti diagnostics [options] [workspace] [paths...]

Print all diagnostics

Options:
  -l, --log-level <logLevel>  Log level to print (choices: "ERROR", "WARN", "INFO", "HINT", default: "WARN")
  -h, --help                  display help for command

From my understanding, VTI can't work without a workspace, right? Because it needs to find the package.json and such things.

Now after the workspace, you can add a list of files and/or directories. Their paths are relative to the workspace.
Then if you want to limit the diagnostics to a file, you also have to pass the workspace:

> vti diagnostics                                 // will run on the current directory as usual, the workspace is still optional
> vti diagnostics . src/App.vue                   // will run only on src/App.vue
> vti diagnostics . src/App.vue src/components/   // will run on src/App.vue and all .vue files inside src/components/
> vti diagnostics src/App.vue                     // will understand src/App.vue as the workspace, and fail

What do you think?

It's well.
@octref Are you have some suggestions?

@gregoirechauvet
Copy link
Contributor Author

Hi. Is there anything I can do to help move this topic forward? 😄
Sorry if I'm being impatient.

@yoyo930021
Copy link
Member

LGTM, I merged it.

@yoyo930021 yoyo930021 merged commit 69cba40 into vuejs:master Jun 1, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow passing files or single files to VTI
3 participants