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

Snap processor makes enforcing other rules on .snap files impossible #292

Closed
scottsheffield opened this issue Jul 5, 2019 · 9 comments · Fixed by #1531
Closed

Snap processor makes enforcing other rules on .snap files impossible #292

scottsheffield opened this issue Jul 5, 2019 · 9 comments · Fixed by #1531

Comments

@scottsheffield
Copy link

Adding the processors to .snap files, and those processors being so explicitly about no-large-snapshots, means I cannot use max-lines, for example, on snapshots to enforce that we are not generating too many snapshots for a given suite.

I appreciate the value of limiting the rules applied to .snap files specifically, but I think it ought to be configurable.

@SimenB
Copy link
Member

SimenB commented Jul 19, 2019

I'm afraid I don't understand the issue here

I think it ought to be configurable.

Configurable in what way?

@scottsheffield
Copy link
Author

If I want to run any specific rules on .snap files, right now they're swallowed by the processor because the processor removes anything that isn't no-large-snapshots.

I'd like to see this written more configurably such that the processor could be bypassed / made optional

@SimenB
Copy link
Member

SimenB commented Aug 7, 2019

Aha, gotcha. Wanna send a PR? I'm not sure how this is normally handled

@G-Rath
Copy link
Collaborator

G-Rath commented Aug 7, 2019

Let me know how you get on w/ this - I have an idea on how to implement a solution, but will leave it for now, as I'm focusing on the TS conversation :)

@G-Rath
Copy link
Collaborator

G-Rath commented Jan 1, 2022

@scottsheffield we've not had anyone else report this, but it does sound reasonable so if you provide me with an easy reproduction I don't mind looking into it further.

Otherwise I'd like to close this issue off.

@G-Rath
Copy link
Collaborator

G-Rath commented May 29, 2022

I was just about to close this but then had a quick look at our processor and saw:

type PostprocessMessage = { ruleId: string };

export const preprocess = (source: string): string[] => [source];
export const postprocess = (messages: PostprocessMessage[][]) =>
  // snapshot files should only be linted with snapshot specific rules
  messages[0].filter(message => message.ruleId === 'jest/no-large-snapshots');

Unfortunately it looks like there's no way to directly configure processors as they don't get anything from the Context (e.g. so we can't read settings):

    export interface LintMessage {
        /**
         * The 1-based column number.
         */
        column: number;
        /**
         * The 1-based column number of the end location.
         */
        endColumn?: number;
        /**
         * The 1-based line number of the end location.
         */
        endLine?: number;
        /**
         * If `true` then this is a fatal error.
         */
        fatal?: true;
        /**
         * Information for autofix.
         */
        fix?: RuleFix;
        /**
         * The 1-based line number.
         */
        line: number;
        /**
         * The error message.
         */
        message: string;
        messageId?: string;
        nodeType: string;
        /**
         * The ID of the rule which makes this message.
         */
        ruleId: string | null;
        /**
         * The severity of this message.
         */
        severity: Severity;
        source: string | null;
        /**
         * Information for suggestions
         */
        suggestions?: LintSuggestion[];
    }

    export interface Processor {
        /**
         * The function to extract code blocks.
         */
        preprocess?: (text: string, filename: string) => Array<string | {
            text: string;
            filename: string;
        }>;
        /**
         * The function to merge messages.
         */
        postprocess?: (messagesList: Linter.LintMessage[][], filename: string) => Linter.LintMessage[];
        /**
         * If `true` then it means the processor supports autofix.
         */
        supportsAutofix?: boolean;
    }

I would say that the solution is we just shouldn't be filtering out rules and that people should be using overrides to apply the processor + specific rules to .snap files but that would mean more setup for everyone wanting to use no-large-snapshots.

Having said that jest/no-large-snapshots is not a recommended rule so it could be fair to say at least that we shouldn't be enabling the processor as part of recommended maybe?

I'm wondering if we actually even need the processor anymore - without that filter all it does is return the sourcecode as-is, which ESLint might do by default for non-js files (wouldn't be surprised if it didn't though 🤷).

@SimenB what do you think?

@G-Rath G-Rath mentioned this issue Aug 27, 2022
@SimenB
Copy link
Member

SimenB commented Aug 28, 2022

Happy to remove it if it's not needed. Give it a go? 😀

Copy link

🎉 This issue has been resolved in version 28.0.0-next.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

Copy link

github-actions bot commented Apr 6, 2024

🎉 This issue has been resolved in version 28.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging a pull request may close this issue.

3 participants