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(rules_check): validate the option examples for rules #4542

Merged
merged 13 commits into from
Nov 15, 2024

Conversation

cr7pt0gr4ph7
Copy link
Contributor

@cr7pt0gr4ph7 cr7pt0gr4ph7 commented Nov 14, 2024

Summary

Extend the rules_check to:

(1) Ensure that configuration fragments are actually valid (and do not contain syntax or schema errors):

    /// ```json,options
    /// {
    ///     "options": { ... }
    /// }
    /// ```

(2) Validate rule examples that use a non-default configuration:

    /// ```json,options
    /// {
    ///     "options": { ... }
    /// }
    /// ```
    ///
    /// Invalid example for this configuration:
    ///
    /// ```js,expect_diagnostic,use_options
    /// import "invalid-import";
    /// ```
    ///
    /// Valid example for this configuration:
    ///
    /// ```json,use_options
    /// import "valid-import";
    /// ```

(3) Allow hiding lines from the output like rustdoc does by prefixing them with # .

The corrresponding PR for biomejs/website is biomejs/website#1442.

Test Plan

  • cargo run -p rules_check
  • I manually inspected the documentation output generated for the website to ensure it is as intended

@github-actions github-actions bot added A-CLI Area: CLI A-Linter Area: linter A-Parser Area: parser A-Tooling Area: internal tools L-JavaScript Language: JavaScript and super languages L-JSON Language: JSON and super languages labels Nov 14, 2024
@cr7pt0gr4ph7 cr7pt0gr4ph7 changed the title Check rule option examples feat(rules_check): validate the option examples for rules Nov 14, 2024
Copy link

codspeed-hq bot commented Nov 14, 2024

CodSpeed Performance Report

Merging #4542 will not alter performance

Comparing cr7pt0gr4ph7:check-rule-option-examples (d5c0c1f) with main (b945258)

Summary

✅ 99 untouched benchmarks

@cr7pt0gr4ph7
Copy link
Contributor Author

Note: This PR would (partially) break the generated documentation until biomejs/website#1442 is merged. I would therefore recommend reviewing this PR here first, then reviewing & merging biomejs/website#1442, and then merging this PR (as outlined in this issue comment).

@cr7pt0gr4ph7
Copy link
Contributor Author

As it turns out, I actually uncovered a bug where implementation & documentation differs for a rule when I un-ignored one of the examples for the use_naming_convention rule:

code-block.ts:2:20 lint/style/useNamingConvention ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

  ! This interface name should be in PascalCase.
  
    1 │ declare module "myExternalModule" {
  > 2 │   export interface my_INTERFACE {}
      │                    ^^^^^^^^^^^^
    3 │ }
    4 │ 
  

Error: Analysis of 'style/useNamingConvention' on the following code block returned an unexpected diagnostic.

This is not actually related to the feature of validating options in the docs, so I have created issue #4545, referenced that issue in the documentation & ignored the offending doctest for now.

Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

Great addition :)

Few questions:

  • how would that work if there are multiple options for a rule, and we want to show multiple examples with different options?
  • is it possible to have multiple option blocks?

crates/biome_analyze/CONTRIBUTING.md Outdated Show resolved Hide resolved
@cr7pt0gr4ph7
Copy link
Contributor Author

Few questions: (1) how would that work if there are multiple options for a rule, and we want to show multiple examples with different options? (2) is it possible to have multiple option blocks?

A [language],use_options block uses the options from the most recently preceding [json_language],options block 1. So the docs should look like this:

  • Example options 1
  • Valid or invalid code example for the given options 1
  • Valid or invalid code example for the given options 1
  • Valid or invalid code example for the given options 1
  • Example options 2
  • Valid or invalid code example for the given options 2
  • etc. ...

A more complete example of a sequence of code blocks (within a single documentation comment):

Block Options Result
js (No Options)
js,use_options Error: No active, valid options block
json,options = Options 1
js,use_options Use Options 1
js (No Options)
js,use_options Use Options 1
json,options Options 2
js,use_options Use Options 2
js,expect_diagnostic,use_options Use Options 2
json,expect_diagnostic,options = Example for Invalid Options
js,use_options Use Options 2 Error: No active, valid options block

This works great for most examples, and encourages the style of simple option example => (code example of that option in action)+.

Further notes:

  • I've only hit some issues for useConsistentMemberAccessibility, where it would probably be cleaner to show multiple diagnostics for a single example of a full class declaration, instead of 6 different but similar examples of members interleaved with their corresponding diagnostics output. Another idea would be to display the diagnostics output in a more compact manner, by making it collapsed-by-default, inlining it into the example code, ...

  • The (not explicitly recommeded by biome_analyze/CONTRIBUTING.md but still followed everywhere) documentation order of Invalid Examples, Valid Examples, Options does not work that well for rules like noRestrictedImports, noRestrictedTypes etc. that only make sense in combination with custom options.

Footnotes

  1. If no options block was encountered yet, or the last options block was a [json_language],expect_diagnostic,options (i.e. an invalid options example - this feature is implemented for the sake of completeness but currently not used), an error is reported on use_options.

@cr7pt0gr4ph7 cr7pt0gr4ph7 force-pushed the check-rule-option-examples branch 3 times, most recently from 356e590 to 26d7cdf Compare November 15, 2024 15:37
@github-actions github-actions bot removed the A-Parser Area: parser label Nov 15, 2024
@ematipico
Copy link
Member

Thank you, @cr7pt0gr4ph7, for the thorough explanation! Let's make sure to have such an explanation in the contribution guide because that's where the majority of our contributors learn how to contribute to our linter :)

@cr7pt0gr4ph7
Copy link
Contributor Author

cr7pt0gr4ph7 commented Nov 15, 2024

Let's make sure to have such an explanation in the contribution guide because that's where the majority of our contributors learn how to contribute to our linter :)

Already done :) See the updated CONTRIBUTING.md. I also gave it a bit of additional polish (grammar, spelling, wording) and tried to make it better navigable for people new to Biome (like me). Is the updated contribution guide complete enough, or should I be even more explicit there?

And also, thank you @arendjr and @ematipico for the quick reactions! It's way more rewarding (and efficient) to contribute to projects where your PRs don't get stalled for 2 months... 👍

@arendjr arendjr merged commit 2491703 into biomejs:main Nov 15, 2024
13 checks passed
@arendjr
Copy link
Contributor

arendjr commented Nov 15, 2024

Thank you, @cr7pt0gr4ph7 ! This is a great improvement.

@Conaclos
Copy link
Member

Thanks! This is a nice addition! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-CLI Area: CLI A-Linter Area: linter A-Tooling Area: internal tools L-JavaScript Language: JavaScript and super languages L-JSON Language: JSON and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants