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(fmt, lint): show number of checked files #7312

Merged
merged 19 commits into from
Sep 9, 2020

Conversation

magurotuna
Copy link
Member

Resolves #6435

This PR adds --verbose (in short, -v) flag to deno fmt and deno lint.

Examples

Given a file named file.ts that is already formatted and have no lint problems:

$ deno lint --unstable --verbose file.ts
Checked 1 file

$ deno lint --unstable -v file.ts
Checked 1 file

$ deno lint --unstable --json -v file.ts
{
  "diagnostics": [],
  "errors": []
}

$ deno fmt --unstable -v file.ts
Checked 1 file

$ deno fmt --unstable --check -v file.ts
Checked 1 file

As you can see, adding -v and --json at the same time is not allowed. If both are present, --json has a priority.

In contrast, if file.ts contains not-formatted code and lint errors:

$ deno lint --unstable -v file.ts
(no-explicit-any) `any` type is not allowed
let a: any = 42
       ~~~
    at /home/yusuktan/Repos/github.com/magurotuna/deno/file.ts:1:7

Found 1 problem
Checked 1 file

$ deno lint --unstable -v --json file.ts
{
  "diagnostics": [
    {
      "location": {
        "line": 1,
        "col": 7
      },
      "filename": "/home/yusuktan/Repos/github.com/magurotuna/deno/file.ts",
      "message": "`any` type is not allowed",
      "code": "no-explicit-any",
      "line_src": "let a: any = 42",
      "snippet_length": 3
    }
  ],
  "errors": []
}

$ deno fmt --unstable -v --check file.ts
from /home/yusuktan/Repos/github.com/magurotuna/deno/file.ts:
1| -let a: any = 42
1| +let a: any = 42;

error: Found 1 not formatted file in 1 file

$ deno fmt --unstable -v file.ts
/home/yusuktan/Repos/github.com/magurotuna/deno/file.ts
Checked 1 file

Any feedback or review would be appreciated, thanks.

cli/flags.rs Outdated Show resolved Hide resolved
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

Thanks @magurotuna, we'll have to land #7303 first and then do the rebase.

cli/flags.rs Outdated Show resolved Hide resolved
@bartlomieju
Copy link
Member

@magurotuna could you rebase the branch?

@bartlomieju bartlomieju added this to the 1.4.0 milestone Sep 2, 2020
@magurotuna
Copy link
Member Author

@bartlomieju
Sure! I'll do soon.

@magurotuna
Copy link
Member Author

In the current implementation, Checked x files goes to stderr, not stdout.
I'm having the second thoughts about it. Is stdout suitable in this case?

@lucacasonato
Copy link
Member

lucacasonato commented Sep 2, 2020

In the current implementation, Checked x files goes to stderr, not stdout.
I'm having the second thoughts about it. Is stdout suitable in this case?

I would prefer stdout here because this message is unconditionally printed regardless if there are errors are not. But if we print everything else on stderr, that might make more sense.

@magurotuna
Copy link
Member Author

@lucacasonato
Thanks, I got it. I'll fix it.

@magurotuna
Copy link
Member Author

Done.
I would be grateful if anyone could re-review it.

cli/flags.rs Outdated Show resolved Hide resolved
cli/fmt.rs Outdated Show resolved Hide resolved
cli/lint.rs Outdated Show resolved Hide resolved
cli/lint.rs Outdated Show resolved Hide resolved
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @magurotuna

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

@magurotuna we have discussed this issue further offline and after all think that adding --verbose flag is not the best way forward. Could you change the PR to show this info by default?

@magurotuna
Copy link
Member Author

@bartlomieju oh, I see. I'll fix it tomorrow(Sep 8th), please wait

@magurotuna
Copy link
Member Author

@bartlomieju I've completed removing verbose. Could you please check it again?

@magurotuna
Copy link
Member Author

Additionally, seeing the info from discord, I updated the available rules in the doc.

@bartlomieju
Copy link
Member

Thanks @magurotuna I'll review shortly, and restart the CI once it's up and running again (https://discord.com/channels/684898665143206084/684911491035430919/752901574077972491)

@bartlomieju bartlomieju changed the title feat(fmt, lint): add verbose flag feat(fmt, lint): show number of checked files Sep 9, 2020
@bartlomieju
Copy link
Member

@magurotuna thanks for the update! I checked the PR locally and I think it's ready to land 👍

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @magurotuna

@bartlomieju bartlomieju merged commit 857f9b3 into denoland:master Sep 9, 2020
@magurotuna magurotuna deleted the add-verbose-flag branch September 9, 2020 15:11
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.

suggestion: deno lint should output the count of checked files
3 participants