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

Ignore directives should affect the entire next AST node #476

Open
nayeemrmn opened this issue Oct 29, 2020 · 7 comments
Open

Ignore directives should affect the entire next AST node #476

nayeemrmn opened this issue Oct 29, 2020 · 7 comments
Labels
feature a new feature

Comments

@nayeemrmn
Copy link
Contributor

As in, the following should be valid:

// deno-lint-ignore no-explicit-any
{
  let a: any;
  let b: any;
  let c: any;
}

This is taken from Rust, #[rustfmt::skip] and #[allow(warnings)] work this way. // deno-fmt-ignore already works this way also. It's much a much nicer way of handling ignore ranges than // deno-lint-ignore-start etc.

@bartlomieju
Copy link
Member

@nayeemrmn very sensible feature, I'm in favor of supporting it. Implementing it will require some refactor to ignore_directive.rs and filtering diagnostics, but both of them were ripe for a cleanup.

@bartlomieju
Copy link
Member

@nayeemrmn I gave this a bit more thought and I'm not so sure anymore - ignoring diagnostic in whole AST means that all diagnostics in children of the node would be ignored as well.

Example:

// deno-lint-ignore no-explicit-any
async function foo(): any {
   
  {
    let a: any;
  }

  ...
}

In above example ignore directive above function would also ignore diagnostics inside block. In real world code this might lead to ignoring very deeply placed diagnostics.

Did I get the idea wrong? Could you elaborate a bit more on the use case?

@nayeemrmn
Copy link
Contributor Author

I considered that, I presume the user would have to do this if they really only wanted to capture the return type:

async function foo(): /* deno-lint-ignore no-explicit-any */ any {
   
  {
    let a: any; // Fails.
  }

  ...
}

If that's unacceptable, I don't mind making exceptions for functions and similar, or even if this proposal only applied to basic
{} blocks.

@bartlomieju
Copy link
Member

I don't like inline block comments, but I think applying to block nodes makes sense.

@RDambrosio016
Copy link

In my opinion this should only be done if the comment is above a statement or module item (this is actually how ignore directives work in rslint 👀). Anything else will give you really weird unpredictable scoping. Then after that you can add deno-lint-ignore-next-line to be more explicit if somebody wants to ignore a whole line. You should reject any directives in that format which are not above a statement

@bartlomieju
Copy link
Member

bartlomieju commented Nov 8, 2020

@RDambrosio016 deno-lint-ignore currently works only on next line. I agree with you on the scoping problem (hence my above comment) but I see value in @nayeemrmn's proposal and I think it does make sense for blocks but only blocks. I really don't want to add more ignore directives, I find it super confusing to have 4 different ignore directives (like eslint does).

@ry ry mentioned this issue Feb 23, 2021
22 tasks
@kdy1
Copy link
Contributor

kdy1 commented Mar 4, 2021

I think the intent of the user is important.

I mean, if ecmascript syntax forces the user to place a node on the specific position, like the return type in the code below,

function foo(
    a: string
): string {
    return a;
}

the ignore directive should go before the ast node, like

// Here
function foo(
    a: string
): string {
    return a;
}

At the same time, I think directives for parameter should be placed before the line because user can change order of parameters.

function foo(
    // Here
    a: string
): string {
    return a;
}

The same rule can be used for block statements.
Block statement is intentional.
I mean, the ecmascript syntax does not force the user to create a block statement in a function body.

function foo() {
    // deno-lint-ignore no-explicit-any
    {
        let a: any;
        let b: any;
        let c: any;
    }
}

The block statement in the function is intentionally created, while the block statement of the function is forced syntax.
So I think the directive should work like

// Does not affect function body.
function foo() {
    // Affect the block statement below.
    {
        let a: any;
        let b: any;
        let c: any;
    }
}

But sometimes a user may want to ignore the node.
I think another directive like deno-lint-ignore-all can be used to support the case.

Alternatively, I think using deno-lint-ignore-all for intentional block statements is also fine.

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

No branches or pull requests

4 participants