Skip to content

Add lint rules - #5

Merged
Jianye Xi (jianyexi) merged 6 commits into
microsoft:mainfrom
jianyexi:add-lint-rule
Oct 26, 2021
Merged

Add lint rules #5
Jianye Xi (jianyexi) merged 6 commits into
microsoft:mainfrom
jianyexi:add-lint-rule

Conversation

@jianyexi

Copy link
Copy Markdown
Contributor

this is split from https://github.com/Azure/cadl-azure/pull/806 , need to commit to the core package first , then update the submodule in the internal repo

Comment thread packages/compiler/core/options.ts Outdated
watchForChanges?: boolean;
serviceCodePath?: string;
/**
* when true , turn off the build check.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So far, I can't figure out from this comment nor the prior internal PR why this is needed. Can you provide more info? My gut feeling is that it's weird to have this option on the CLI, but maybe I'm missing something.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh, this tells linters to skip checks, it doesn't skip the compiler's checker (which would be very weird but what I thought from the naming and a misread of the other PR). I think we should avoid using the terms check/checker for extra linting checks and keep it for compiler's critical analysis.

@timotheeguerin Timothee Guerin (timotheeguerin) Oct 21, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

yeah so this is actually maybe a bit weird as we don't have the concept of a linter anymore. Where do we draw the line where this flag would disable certain check?
Would it be that all the warning check or some decision on a per diagnostic basis.
If the 1st one maybe we can name --ignoreWarnings or something like that instead and still run all the checker but ignore the warnings at the end
If the 2nd one maybe have something like --skipOptionalChecks but should we really let that be disabled

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This feels more like it should be a setting for the level of reported checks (e.g. error, warning, info, debug), where essentially you report diagnostics at that level or of higher severity, with default to the lowest severity setting (or, perhaps, info), something like --diagnosticLevel

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This option if for skipping the lint checks which will be ran in the onBuild of emitter,
I am totally agreed to use the --diagnosticLevel to specify which diagnostics should be reported, as now all the lint rules are warnings .

@nguerrera Nick Guerrera (nguerrera) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good. Small request: could you squash and merge this and change the commit description while doing so to say what is changing in this OSS repo (instead of "add lint rules" which is not actually what this change does when looked at alone in this repo).

@jianyexi
Jianye Xi (jianyexi) merged commit 37c4e3a into microsoft:main Oct 26, 2021
Timothee Guerin (timotheeguerin) referenced this pull request in timotheeguerin/typespec Jan 4, 2022
* add  option `--diagnostic-level` to specify which level of messages should be reported
* add docs for built-in types
* improve & export semantic-walk lib 

Co-authored-by: jianye xi <jianyxi@microsoft.com>
Patrick Hallisey (hallipr) referenced this pull request in hallipr/typespec Apr 11, 2024
Weidong Xu (weidongxu-microsoft) pushed a commit to weidongxu-microsoft/typespec that referenced this pull request Oct 22, 2024
Christopher Radek (chrisradek) added a commit that referenced this pull request Dec 3, 2024
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.

4 participants