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(biome_cli): add schema version check to check,lint,format commands #4796

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

MaxtuneLee
Copy link
Contributor

@MaxtuneLee MaxtuneLee commented Dec 26, 2024

Summary

Issue mentioned in #4147

Add function check_schema_version which will compare version and print warning information.

Test Plan

  • Update tests to ensure no warning is issued when the versions match.
  • Add tests to check for schema version mismatches in the check, format, and lint commands.

@github-actions github-actions bot added the A-CLI Area: CLI label Dec 26, 2024
@MaxtuneLee
Copy link
Contributor Author

The test action failed as expected because the test was built without the BIOME_VERSION variable, so the biome version inside is the default value '0.0.0', which does not match the schema version. I'm wondering whether I should update the snapshot or add the variable to the action.

---- commands::check::should_show_formatter_diagnostics_for_files_ignored_by_linter stdout ----
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ Snapshot Summary ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
Snapshot file: crates/biome_cli/tests/snapshots/main_commands_check/should_show_formatter_diagnostics_for_files_ignored_by_linter.snap
Snapshot: should_show_formatter_diagnostics_for_files_ignored_by_linter
Source: crates/biome_cli/tests/snap_test.rs:423
────────────────────────────────────────────────────────────────────────────────
Expression: content
────────────────────────────────────────────────────────────────────────────────
-old snapshot
+new results
────────────┬───────────────────────────────────────────────────────────────────
   36    36 │ 
   37    37 │ # Emitted Messages
   38    38 │ 
   39    39 │ ```block
         40 │+The configuration schema version does not match the CLI version.
         41 │+  Expect:                       0.0.0
         42 │+  Found:                        1.6.1
         43 │+
         44 │+If you wish to update the configuration schema, setting the `$schema` option to the expected version.
         45 │+```
         46 │+
         47 │+```block
   40    48 │ build/file.js format ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
   41    49 │ 
   42    50 │   × Formatter would have printed the following content:
   43    51 │   
────────────┴───────────────────────────────────────────────────────────────────
Stopped on the first failure. Run `cargo insta test` to run all snapshots.

@dyc3
Copy link
Contributor

dyc3 commented Dec 26, 2024

To minimize what our existing tests are asserting for, I think it would be better to change the schema url in the sample config for that test to 0.0.0. You should also add a new snapshot test for this warning.

@github-actions github-actions bot added the A-Changelog Area: changelog label Dec 27, 2024
@MaxtuneLee MaxtuneLee marked this pull request as ready for review December 27, 2024 06:43
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.

Thank you for your contribution!

While the logic is correct, unfortunately it isn't in the right place. This was a mistake on our parts when we described the issue. The problem is, we described the solution only by focusing on the CLI as our only client, but there are clients like the LSP.

We should place the version check logic when we deserialize the configuration.

So, this is where our configuration is defined

https://github.com/biomejs/biome/blob/main/crates%2Fbiome_configuration%2Fsrc%2Flib.rs#L83-L86

You'll have to create a new type e.g. Schema(String), very similar to this code

https://github.com/biomejs/biome/blob/main/crates%2Fbiome_deserialize%2Fsrc%2Fstring_set.rs#L10-L13

And then implement a custom deserialization, where you will place the logic you just added. You can check this example

https://github.com/biomejs/biome/blob/main/crates%2Fbiome_project%2Fsrc%2Fnode_js_project%2Fpackage_json.rs#L149-L162

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Changelog Area: changelog A-CLI Area: CLI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants