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

feat: Verbose as CLI flag #319

Closed

Conversation

montogeek
Copy link

As discussed on #126, I added verbose mode as CLI flag.
I am not sure about variables naming, but it works and tests pass.
I think it need more tests, let me know if should be necessary.

@@ -36,9 +36,14 @@ module.exports = function lintStaged(injectedLogger, configPath) {
.then(result => {
if (result == null) throw errConfigNotFound

const configResult = {
verbose,
...result.config
Copy link
Collaborator

Choose a reason for hiding this comment

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

We are not transpiling the source code and destructuring objects isn't supported in node v4 and v6. Additionally, at this point the result.config isn't normalized. So it could be in the simple or advanced format. You can refer https://github.com/okonet/lint-staged#configuration to know more about configuration. If the type is simple, then adding the verbose flag to it would be incorrect.

@okonet
Copy link
Collaborator

okonet commented Oct 28, 2017

@montogeek do you mind fixing the PR? I'd love to merge it!

@montogeek
Copy link
Author

@sudo-suhas I do not understand what you mean, if the configuration have verbose it is advanced? So it means verbose CLI flag should be added when being normalized?

@sudo-suhas
Copy link
Collaborator

@montogeek Sorry I wasn't clear. So here's an example of a simple config:

{
  "*.js": ["my-cmd"]
}

And an advanced one:

{
  "linters": {
    "*.js": ["my-cmd"]
  },
  "verbose": true
}

The simple one is converted to the advanced format in getConfig.js:
https://github.com/okonet/lint-staged/blob/331d3c5773b8013892a903bfd35eb8634db85741/src/getConfig.js#L94-L99

And this would be the right place to introduce the verbose flag if we got it in the command line args.
Since we use the config.verbose for determining the renderer, we should use the cmd-line flag before that:

------------------------------- src/getConfig.js -------------------------------
index 7d0a0bb..e087ed9 100644
@@ -91,9 +91,9 @@ function unknownValidationReporter(config, example, option, options) {
  *  concurrent: boolean, chunkSize: number, gitDir: string, globOptions: {matchBase: boolean, dot: boolean}, linters: {}, subTaskConcurrency: number, renderer: string, verbose: boolean
  * }}
  */
-function getConfig(sourceConfig) {
+function getConfig(sourceConfig, verbose) {
   const config = defaultsDeep(
-    {}, // Do not mutate sourceConfig!!!
+    { verbose }, // Do not mutate sourceConfig!!!
     isSimple(sourceConfig) ? { linters: sourceConfig } : sourceConfig,
     defaultConfig
   )

This should also ensure that the command line flag overrides config if applicable.

@okonet
Copy link
Collaborator

okonet commented Nov 13, 2017

@montogeek any updates on this one? It would be really great to have this. I'd also suggest to remove the config option verbose along with adding a deprecation message for it like here: https://github.com/okonet/lint-staged/pull/327/files#diff-c90c061ebd15a26a15830f7fcc0c7544R122

@sudo-suhas
Copy link
Collaborator

@okonet Why not support both?

@okonet
Copy link
Collaborator

okonet commented Nov 16, 2017

@sudo-suhas I think it doesn't belong to the config, TBH. verbose is something you use when you have problems, not all the time. Am I missing some use cases?

@sudo-suhas
Copy link
Collaborator

What you are saying makes sense. But I have in fact, used the verbose flag in the config on occasion(with ava).

@okonet okonet mentioned this pull request Nov 27, 2017
@montogeek montogeek deleted the feature/verbose-as-cli-flag branch December 1, 2017 09:14
@okonet okonet mentioned this pull request Jan 21, 2018
7 tasks
@dhoko dhoko mentioned this pull request May 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants