Handle lint level in check_release #805

suaviloquence · 2024-06-26T20:18:25Z

trying something new with smaller, annotated commits, lmk if it is helpful or not.

This PR handles lint level in check_release, especially differentiating warnings from errors. It now skips running any checks marked as allow, prints errors and warnings differently, and the summary now reflects the required version bump (from error/deny checks) and the suggested version bump (from warning checks, if it is higher than the required bump).

example command output

$ cargo r -- semver-checks --baseline-root old --manifest-path new/pkg/Cargo.toml -v
   Compiling cargo-semver-checks v0.32.0 (/home/m/dev/cargo-semver-checks)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 4.72s
     Running `/home/m/dev/cargo-semver-checks/target/debug/cargo-semver-checks semver-checks --baseline-root old --manifest-path new/pkg/Cargo.toml -v`
     Parsing pkg v0.1.0 (current)
      Parsed [   0.472s] (current)
     Parsing pkg v0.1.0 (baseline)
      Parsed [   0.460s] (baseline)
    Checking pkg v0.1.0 -> v0.1.0 (no change)
    Starting 75 checks, 0 unnecessary on 16 threads
        PASS [   0.002s]       major            deny     auto_trait_impl_removed
        PASS [   0.001s]       major            deny     constructible_struct_adds_field
        PASS [   0.001s]       major            deny     constructible_struct_adds_private_field
        PASS [   0.001s]       major            deny     constructible_struct_changed_type
        PASS [   0.002s]       major            deny     derive_trait_impl_removed
        PASS [   0.001s]       major            deny     enum_marked_non_exhaustive
        PASS [   0.000s]       major            deny     enum_missing
        PASS [   0.001s]       minor            deny     enum_must_use_added
        PASS [   0.001s]       major            deny     enum_now_doc_hidden
        PASS [   0.002s]       major            deny     enum_repr_int_changed
        PASS [   0.001s]       major            deny     enum_repr_int_removed
        PASS [   0.001s]       major            deny     enum_repr_transparent_removed
        PASS [   0.001s]       major            deny     enum_struct_variant_field_added
        PASS [   0.001s]       major            deny     enum_struct_variant_field_missing
        PASS [   0.001s]       major            deny     enum_struct_variant_field_now_doc_hidden
        PASS [   0.001s]       major            deny     enum_tuple_variant_field_added
        PASS [   0.001s]       major            deny     enum_tuple_variant_field_missing
        PASS [   0.001s]       major            deny     enum_tuple_variant_field_now_doc_hidden
        PASS [   0.001s]       major            deny     enum_variant_added
        PASS [   0.001s]       major            deny     enum_variant_missing
        PASS [   0.000s]       major            deny     exported_function_changed_abi
        PASS [   0.001s]       major            deny     function_abi_no_longer_unwind
        PASS [   0.001s]       major            deny     function_changed_abi
        PASS [   0.000s]       major            deny     function_const_removed
        PASS [   0.000s]       major            deny     function_export_name_changed
        FAIL [   0.001s]       minor            deny     function_missing
        PASS [   0.001s]       minor            deny     function_must_use_added
        PASS [   0.001s]       major            deny     function_now_doc_hidden
        PASS [   0.001s]       major            deny     function_parameter_count_changed
        PASS [   0.001s]       major            deny     function_unsafe_added
        PASS [   0.001s]       major            deny     inherent_associated_pub_const_missing
        PASS [   0.001s]       major            deny     inherent_method_const_removed
        PASS [   0.001s]       major            deny     inherent_method_missing
        PASS [   0.002s]       minor            deny     inherent_method_must_use_added
        PASS [   0.001s]       major            deny     inherent_method_unsafe_added
        PASS [   0.002s]       major            deny     method_parameter_count_changed
        FAIL [   0.001s]       major            warn     module_missing
        PASS [   0.001s]       major            deny     pub_module_level_const_missing
        PASS [   0.001s]       major            deny     pub_module_level_const_now_doc_hidden
        PASS [   0.001s]       major            deny     pub_static_missing
        PASS [   0.001s]       major            deny     pub_static_mut_now_immutable
        PASS [   0.001s]       major            deny     pub_static_now_doc_hidden
        PASS [   0.001s]       major            deny     repr_c_removed
        PASS [   0.001s]       major            deny     repr_packed_added
        PASS [   0.001s]       major            deny     repr_packed_removed
        PASS [   0.001s]       major            deny     sized_impl_removed
        PASS [   0.002s]       major            deny     struct_marked_non_exhaustive
        PASS [   0.001s]       major            deny     struct_missing
        PASS [   0.001s]       minor            deny     struct_must_use_added
        PASS [   0.001s]       major            deny     struct_now_doc_hidden
        PASS [   0.001s]       major            deny     struct_pub_field_missing
        PASS [   0.001s]       major            deny     struct_pub_field_now_doc_hidden
        PASS [   0.001s]       major            deny     struct_repr_transparent_removed
        PASS [   0.002s]       major            deny     struct_with_pub_fields_changed_type
        PASS [   0.001s]       major            deny     trait_associated_const_now_doc_hidden
        PASS [   0.001s]       major            deny     trait_associated_type_now_doc_hidden
        PASS [   0.001s]       major            deny     trait_method_missing
        PASS [   0.001s]       major            deny     trait_method_now_doc_hidden
        PASS [   0.003s]       major            deny     trait_method_unsafe_added
        PASS [   0.001s]       major            deny     trait_method_unsafe_removed
        PASS [   0.001s]       major            deny     trait_missing
        PASS [   0.001s]       minor            deny     trait_must_use_added
        PASS [   0.001s]       major            deny     trait_now_doc_hidden
        PASS [   0.002s]       major            deny     trait_removed_associated_constant
        PASS [   0.001s]       major            deny     trait_removed_associated_type
        PASS [   0.002s]       major            deny     trait_removed_supertrait
        PASS [   0.001s]       major            deny     trait_unsafe_added
        PASS [   0.001s]       major            deny     trait_unsafe_removed
        PASS [   0.001s]       major            deny     tuple_struct_to_plain_struct
        PASS [   0.001s]       minor            deny     type_marked_deprecated
        PASS [   0.001s]       major            deny     union_field_missing
        PASS [   0.000s]       major            deny     union_missing
        PASS [   0.000s]       major            deny     union_now_doc_hidden
        PASS [   0.000s]       major            deny     unit_struct_changed_kind
        PASS [   0.001s]       major            deny     variant_marked_non_exhaustive
     Checked [   0.012s] 75 checks; 73 passed, 2 failed, 0 unnecessary

--- failure function_missing: pub fn removed or renamed ---

Description:
A publicly-visible function cannot be imported by its prior path. A `pub use` may have been removed, or the function itself may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.32.0/src/lints/function_missing.ron

Failed in:
  function pkg::function_missing, previously in file /home/m/dev/cargo-semver-checks/test_crates/manifest_tests/workspace_overrides/old/pkg/src/lib.rs:5

--- warning: module_missing: pub module removed or renamed ---

Description:
A publicly-visible module cannot be imported by its prior path. A `pub use` may have been removed, or the module may have been renamed, removed, or made non-public.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.32.0/src/lints/module_missing.ron

Failed in:
  mod pkg::module_missing, previously in file /home/m/dev/cargo-semver-checks/test_crates/manifest_tests/workspace_overrides/old/pkg/src/lib.rs:9
warning: warning checks suggest new major version
warning: 1 major and 0 minor warning checks failed
     Summary semver requires new minor version: 0 major and 1 minor checks failed
    Finished [   0.961s] pkg

Commits:

skip queries that are configured less than warn
print lint level for each check in debug log line
derive Ord on RequiredSemverUpdate
split query results into warnings and errors on failure
print warnings before errors and print summary with warnings
change wording to be more consistent between warnings/errors
update workspace test

In addition to adding more tests, one other thing to work on is increasing the consistency in formatted text, if we want it to be similar to tools like clippy, for instance. I tried to make it internally consistent between warnings/errors, but the program has certain behaviors such as printing some lint failure info to stdout and some to stderr that we could fix by following what linters like clippy do, if we want that.

- if we ever want an --ignore-warnings (or --ignore-less-than LEVEL) flag, we can change the hardcoded `LintLevel::Warn` to something else - should add a test to verify correct behavior

- will be useful for tests without new API - the 12 chars is kind of arbitrary but 18 chars looked too wide

- so we can call `.max()` on the `Vec` and get an `Option<update>` - it needs to be an `Option` now because there may be nothing (e.g., only warnings fail)

- follow clippy's behavior of printing individual errors before warnings, but printing warning summary before error summary - always print number of warning checks that fail, if there are any, but only print suggested version bump if it is greater than required version bump. - refactor a little to use `max` instead of `contains` when calculating the required/suggested bump

obi1kenobi

Designing CLI output is really hard: there are lots of competing objectives and it isn't always clear what the answer should be.

I'd recommend stepping away from the Rust code, and just taking some time to look at and design the printout of running the tool in various cases: when it had warnings, when it didn't have warnings, when some config was overridden (and where from!), etc.

Consider what the user would probably want to know at each stage, what additional info they might know in --verbose mode, and what info isn't useful and can be omitted.

Consider that they are often reading the output on a terminal that's only ~75 chars wide, where wrapping text can often look really ugly unless it's specifically meant to be a paragraph and spaced accordingly.

After you have some designs you are happy with, let's review just the mock printouts for each case — no implementation yet! That will make it easier for us to make small improvements in an iterative fashion, without forcing you to constantly adjust the implementation.

In the meantime, if you want to merge the parts of this PR that don't affect the CLI output, you could split them off into a separate PR that we could merge. But that might not be worth the effort given it's just a dozen lines or so — your call.

src/check_release.rs

obi1kenobi · 2024-06-26T21:04:37Z

src/check_release.rs

-                    queries_to_run.len() - results_with_errors.len(),
-                    results_with_errors.len(),
+                    queries_to_run.len() - results_with_errors.len() - results_with_warnings.len(),
+                    results_with_errors.len() + results_with_warnings.len(),


Might we want to separate out "failed" from "had warnings" here? Saying something failed but then exiting cleanly feels strange...

obi1kenobi · 2024-06-26T21:06:50Z

src/check_release.rs

                if results.is_empty() {
                    writeln!(
                        config.stderr(),
-                        "{}{:>12}{} [{:8.3}s] {:^18} {}",
+                        "{}{:>12}{} [{:8.3}s] {:^18} {:^12} {}",


I think this results in output that is too wide. Of the ~75 chars we have to play with, we've already spent almost 50 before we even print the lint name. That's too much.

Perhaps we can combine the category and lint level fields into one table column somehow?

obi1kenobi · 2024-06-26T21:08:29Z

src/check_release.rs


-        config
-            .shell_print(
+            print_failed_lint(config, semver_query, results)?;


This function might be a good candidate to rename, given its new role in both warnings and failures.

obi1kenobi · 2024-06-26T21:09:18Z

src/check_release.rs

+                config.shell_warn(format_args!(
+                    "warning checks suggest new {} version",
+                    suggested_bump.as_str()
+                ))?;


This produces text that's like warning: warning checks ...

It feels like it could use another polishing pass.

obi1kenobi · 2024-06-26T21:10:00Z

src/check_release.rs

+            }
+
+            config.shell_warn(format_args!(
+                "{} major and {} minor warning checks failed",


It's strange to say "warning checks failed."

Perhaps "produced warnings" or "triggered warnings" or something to that effect?

obi1kenobi · 2024-06-26T21:11:20Z

src/check_release.rs

+            ))?;
+        }
+
+        if let Some(required_bump) = required_bump {


It's strange that by this point, we've already printed out summary information about warnings, and now we print a line titled "Summary" with more information.

obi1kenobi · 2024-06-26T21:30:45Z

trying something new with smaller, annotated commits, lmk if it is helpful or not.

So long as they are in the same PR, it usually wouldn't be particularly more helpful to me since the GitHub workflow shows a squashed diff by default. It might sometimes be more useful if I need to dig into the changes for a particular commit, but honestly if I have to dig that deep, it probably either should have been its own PR instead or there's some sort of code quality issue that shouldn't have been there in the first place.

I certainly don't mind it and if you like it, you're welcome to keep using it. But if it's extra work for you, then it's probably not worth it.

In addition to adding more tests, one other thing to work on is increasing the consistency in formatted text, if we want it to be similar to tools like clippy, for instance. I tried to make it internally consistent between warnings/errors, but the program has certain behaviors such as printing some lint failure info to stdout and some to stderr that we could fix by following what linters like clippy do, if we want that.

Yes, it would be great to have more consistency across the entire CLI experience, and I welcome the help.

The cargo-semver-checks output formatting at the moment is inspired by cargo-nextest, not clippy. I don't have strong feelings about which we should follow, and I could be swayed by a convincing argument either way.

The top thing I'd worry about is a half-completed attempt to change from nextest-style to clippy-style output, because that'd spend a bunch of our energy while leaving us in the middle of nowhere.

And I'd certainly love the help if you'd like to think through, formalize, write down, and apply new rules for which things go on stderr vs stdout :)

suaviloquence · 2024-06-27T20:14:22Z

Some thoughts:

And I'd certainly love the help if you'd like to think through, formalize, write down, and apply new rules for which things go on stderr vs stdout :)

Playing around with both nextest and clippy, both tools put everything I tested (except for --help messages) in stderr. cargo-semver-checks writes most things to stderr, but when a check fails, it prints the lint name/description/info/span to stdout. Is there a reason we print just this to stdout, or would it make sense to move this to stderr?

The cargo-semver-checks output formatting at the moment is inspired by cargo-nextest, not clippy. I don't have strong feelings about which we should follow, and I could be swayed by a convincing argument either way.

nextest, as a test runner instead of a linter, doesn't seem to have the concept of a warning. We can definitely keep using this format, but we would need to define both how a warning is displayed when a warning-level lint procs, and the order that errors and warnings are printed, both individually and as a summary.

clippy screenshot

clippy prints individual errors before warnings, but the warning summary before the error summary. We could do something similar. It might also be helpful to track the configuration origin in a note: like clippy does, although that doesn't need to be added immediately.

Here's one nextest-inspired idea: print the FAIL [ ###s] semver id always on fail, and similarly with WARN on a triggered warning. Currently, these are only printed in --verbose mode, but with this idea, we always print failures/warnings (passed checks still only with --verbose) and remove the current --- failure: ... --- lines

                                                                        75|
     Parsing pkg v0.1.0 (current)
      Parsed [   0.466s] (current)
     Parsing pkg v0.1.0 (baseline)
      Parsed [   0.457s] (baseline)
    Checking pkg v0.1.0 -> v0.1.0 (no change)
     Checked [   0.007s] 75 checks; 73 passed, 1 failed, 1 warning, 0 unnecessary
    FAIL [   0.001s]       minor        function_missing
  pub fn removed or renamed  // (maybe an error: prefix? with config.shell_error())    

Description:
A publicly-visible function cannot be imported by its prior path. A `pub use` may have been removed, or the function itself may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.32.0/src/lints/function_missing.ron
Failed in:
  function pkg::function_missing, previously in file /home/m/dev/cargo-semver-checks/test_crates/manifest_tests/workspace_overrides/old/pkg/src/lib.rs:5

    WARN [   0.001s]       major        module_missing
  pub module renamed or removed  // (warning: prefix? with config.shell_warn())
Description:
A publicly-visible module cannot be imported by its prior path. A `pub use` may have been removed, or the module may have been renamed, removed, or made non-public.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.32.0/src/lints/module_missing.ron

Failed in:
  mod pkg::module_missing, previously in file /home/m/dev/cargo-semver-checks/test_crates/manifest_tests/workspace_overrides/old/pkg/src/lib.rs:9

note: requested by `function_missing = "warn"` in [package.metadata]  // optional
   Summary 0 major and 1 minor checks failed
    Warning 1 major- and 0 minor-level warnings produced // only print if nonzero
            Warning checks suggest new major version // only print if > required
    Failure semver requires new minor version
    Success no version bump required // on no deny-level checks failing
   Finished [   0.958s] pkg

Thoughts on this? The summary section is a little tricky (esp. heading names) to handle all of the different cases.

Co-authored-by: Predrag Gruevski <[email protected]>

- show PASS/FAIL/WARN in verbose mode - print `--- failure ---` or `--- warning ---` on triggered lint - update summary - add new test for when only warnings are produced

suaviloquence · 2024-07-02T23:30:27Z

New formatting - still don't think the summary is perfect, lmk if you have suggestions.

Just warnings, no errors

     Running `/home/m/dev/cargo-semver-checks/target/debug/cargo-semver-checks semver-checks --baseline-root old --manifest-path new/pkg -v`
     Parsing only-warnings v0.1.0 (current)
      Parsed [   0.346s] (current)
     Parsing only-warnings v0.1.0 (baseline)
      Parsed [   0.344s] (baseline)
    Checking only-warnings v0.1.0 -> v0.1.0 (no change)
    Starting 75 checks, 0 unnecessary on 16 threads
        PASS [   0.001s]       major        auto_trait_impl_removed
        PASS [   0.001s]       major        constructible_struct_adds_field
        PASS [   0.001s]       major        constructible_struct_adds_private_field
        PASS [   0.000s]       major        constructible_struct_changed_type
        PASS [   0.001s]       major        derive_trait_impl_removed
        PASS [   0.001s]       major        enum_marked_non_exhaustive
        PASS [   0.001s]       major        enum_missing
        PASS [   0.001s]       minor        enum_must_use_added
        PASS [   0.000s]       major        enum_now_doc_hidden
        PASS [   0.001s]       major        enum_repr_int_changed
        PASS [   0.001s]       major        enum_repr_int_removed
        PASS [   0.001s]       major        enum_repr_transparent_removed
        PASS [   0.001s]       major        enum_struct_variant_field_added
        PASS [   0.001s]       major        enum_struct_variant_field_missing
        PASS [   0.001s]       major        enum_struct_variant_field_now_doc_hidden
        PASS [   0.001s]       major        enum_tuple_variant_field_added
        PASS [   0.001s]       major        enum_tuple_variant_field_missing
        PASS [   0.001s]       major        enum_tuple_variant_field_now_doc_hidden
        PASS [   0.001s]       major        enum_variant_added
        PASS [   0.000s]       major        enum_variant_missing
        PASS [   0.000s]       major        exported_function_changed_abi
        PASS [   0.000s]       major        function_abi_no_longer_unwind
        PASS [   0.001s]       major        function_changed_abi
        PASS [   0.000s]       major        function_const_removed
        PASS [   0.000s]       major        function_export_name_changed
        WARN [   0.001s]       major        function_missing
        PASS [   0.001s]       minor        function_must_use_added
        PASS [   0.001s]       major        function_now_doc_hidden
        PASS [   0.001s]       major        function_parameter_count_changed
        PASS [   0.000s]       major        function_unsafe_added
        PASS [   0.001s]       major        inherent_associated_pub_const_missing
        PASS [   0.001s]       major        inherent_method_const_removed
        PASS [   0.001s]       major        inherent_method_missing
        PASS [   0.001s]       minor        inherent_method_must_use_added
        PASS [   0.001s]       major        inherent_method_unsafe_added
        PASS [   0.001s]       major        method_parameter_count_changed
        PASS [   0.000s]       major        module_missing
        PASS [   0.001s]       major        pub_module_level_const_missing
        PASS [   0.001s]       major        pub_module_level_const_now_doc_hidden
        PASS [   0.001s]       major        pub_static_missing
        PASS [   0.001s]       major        pub_static_mut_now_immutable
        PASS [   0.001s]       major        pub_static_now_doc_hidden
        PASS [   0.001s]       major        repr_c_removed
        PASS [   0.001s]       major        repr_packed_added
        PASS [   0.001s]       major        repr_packed_removed
        PASS [   0.001s]       major        sized_impl_removed
        PASS [   0.001s]       major        struct_marked_non_exhaustive
        PASS [   0.000s]       major        struct_missing
        PASS [   0.000s]       minor        struct_must_use_added
        PASS [   0.000s]       major        struct_now_doc_hidden
        PASS [   0.000s]       major        struct_pub_field_missing
        PASS [   0.000s]       major        struct_pub_field_now_doc_hidden
        PASS [   0.001s]       major        struct_repr_transparent_removed
        PASS [   0.001s]       major        struct_with_pub_fields_changed_type
        PASS [   0.000s]       major        trait_associated_const_now_doc_hidden
        PASS [   0.000s]       major        trait_associated_type_now_doc_hidden
        PASS [   0.001s]       major        trait_method_missing
        PASS [   0.000s]       major        trait_method_now_doc_hidden
        PASS [   0.000s]       major        trait_method_unsafe_added
        PASS [   0.001s]       major        trait_method_unsafe_removed
        PASS [   0.000s]       major        trait_missing
        PASS [   0.000s]       minor        trait_must_use_added
        PASS [   0.000s]       major        trait_now_doc_hidden
        PASS [   0.000s]       major        trait_removed_associated_constant
        PASS [   0.000s]       major        trait_removed_associated_type
        PASS [   0.001s]       major        trait_removed_supertrait
        PASS [   0.000s]       major        trait_unsafe_added
        PASS [   0.000s]       major        trait_unsafe_removed
        PASS [   0.001s]       major        tuple_struct_to_plain_struct
        PASS [   0.000s]       minor        type_marked_deprecated
        PASS [   0.001s]       major        union_field_missing
        PASS [   0.000s]       major        union_missing
        PASS [   0.000s]       major        union_now_doc_hidden
        PASS [   0.000s]       major        unit_struct_changed_kind
        PASS [   0.001s]       major        variant_marked_non_exhaustive
     Checked [   0.007s] 75 checks; 74 passed, 0 failed, 1 warnings, 0 skipped

--- warning function_missing: pub fn removed or renamed ---

Description:
A publicly-visible function cannot be imported by its prior path. A `pub use` may have been removed, or the function itself may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.32.0/src/lints/function_missing.ron

Failed in:
  function only_warnings::function_missing, previously in file /home/m/dev/cargo-semver-checks/test_crates/manifest_tests/only_warnings/old/src/lib.rs:4
     Summary no semver update required
     Warning produced 1 major and 0 minor level warnings
             produced warnings suggest new major version
    Finished [   0.706s] only-warnings

Both warnings and errors

     Running `/home/m/dev/cargo-semver-checks/target/debug/cargo-semver-checks semver-checks --baseline-root old --manifest-path new/pkg -v`
     Parsing pkg v0.1.0 (current)
      Parsed [   0.331s] (current)
     Parsing pkg v0.1.0 (baseline)
      Parsed [   0.325s] (baseline)
    Checking pkg v0.1.0 -> v0.1.0 (no change)
    Starting 75 checks, 0 unnecessary on 16 threads
        PASS [   0.001s]       major        auto_trait_impl_removed
        PASS [   0.001s]       major        constructible_struct_adds_field
        PASS [   0.001s]       major        constructible_struct_adds_private_field
        PASS [   0.001s]       major        constructible_struct_changed_type
        PASS [   0.001s]       major        derive_trait_impl_removed
        PASS [   0.000s]       major        enum_marked_non_exhaustive
        PASS [   0.001s]       major        enum_missing
        PASS [   0.001s]       minor        enum_must_use_added
        PASS [   0.000s]       major        enum_now_doc_hidden
        PASS [   0.001s]       major        enum_repr_int_changed
        PASS [   0.001s]       major        enum_repr_int_removed
        PASS [   0.001s]       major        enum_repr_transparent_removed
        PASS [   0.001s]       major        enum_struct_variant_field_added
        PASS [   0.001s]       major        enum_struct_variant_field_missing
        PASS [   0.001s]       major        enum_struct_variant_field_now_doc_hidden
        PASS [   0.001s]       major        enum_tuple_variant_field_added
        PASS [   0.001s]       major        enum_tuple_variant_field_missing
        PASS [   0.001s]       major        enum_tuple_variant_field_now_doc_hidden
        PASS [   0.001s]       major        enum_variant_added
        PASS [   0.000s]       major        enum_variant_missing
        PASS [   0.000s]       major        exported_function_changed_abi
        PASS [   0.000s]       major        function_abi_no_longer_unwind
        PASS [   0.001s]       major        function_changed_abi
        PASS [   0.001s]       major        function_const_removed
        PASS [   0.000s]       major        function_export_name_changed
        FAIL [   0.000s]       minor        function_missing
        PASS [   0.001s]       minor        function_must_use_added
        PASS [   0.001s]       major        function_now_doc_hidden
        PASS [   0.001s]       major        function_parameter_count_changed
        PASS [   0.000s]       major        function_unsafe_added
        PASS [   0.001s]       major        inherent_associated_pub_const_missing
        PASS [   0.001s]       major        inherent_method_const_removed
        PASS [   0.001s]       major        inherent_method_missing
        PASS [   0.001s]       minor        inherent_method_must_use_added
        PASS [   0.001s]       major        inherent_method_unsafe_added
        PASS [   0.001s]       major        method_parameter_count_changed
        WARN [   0.001s]       major        module_missing
        PASS [   0.001s]       major        pub_module_level_const_missing
        PASS [   0.000s]       major        pub_module_level_const_now_doc_hidden
        PASS [   0.001s]       major        pub_static_missing
        PASS [   0.001s]       major        pub_static_mut_now_immutable
        PASS [   0.001s]       major        pub_static_now_doc_hidden
        PASS [   0.001s]       major        repr_c_removed
        PASS [   0.001s]       major        repr_packed_added
        PASS [   0.001s]       major        repr_packed_removed
        PASS [   0.001s]       major        sized_impl_removed
        PASS [   0.001s]       major        struct_marked_non_exhaustive
        PASS [   0.000s]       major        struct_missing
        PASS [   0.000s]       minor        struct_must_use_added
        PASS [   0.000s]       major        struct_now_doc_hidden
        PASS [   0.001s]       major        struct_pub_field_missing
        PASS [   0.000s]       major        struct_pub_field_now_doc_hidden
        PASS [   0.001s]       major        struct_repr_transparent_removed
        PASS [   0.001s]       major        struct_with_pub_fields_changed_type
        PASS [   0.000s]       major        trait_associated_const_now_doc_hidden
        PASS [   0.000s]       major        trait_associated_type_now_doc_hidden
        PASS [   0.001s]       major        trait_method_missing
        PASS [   0.001s]       major        trait_method_now_doc_hidden
        PASS [   0.001s]       major        trait_method_unsafe_added
        PASS [   0.001s]       major        trait_method_unsafe_removed
        PASS [   0.000s]       major        trait_missing
        PASS [   0.001s]       minor        trait_must_use_added
        PASS [   0.000s]       major        trait_now_doc_hidden
        PASS [   0.001s]       major        trait_removed_associated_constant
        PASS [   0.000s]       major        trait_removed_associated_type
        PASS [   0.001s]       major        trait_removed_supertrait
        PASS [   0.000s]       major        trait_unsafe_added
        PASS [   0.000s]       major        trait_unsafe_removed
        PASS [   0.001s]       major        tuple_struct_to_plain_struct
        PASS [   0.000s]       minor        type_marked_deprecated
        PASS [   0.001s]       major        union_field_missing
        PASS [   0.000s]       major        union_missing
        PASS [   0.000s]       major        union_now_doc_hidden
        PASS [   0.000s]       major        unit_struct_changed_kind
        PASS [   0.001s]       major        variant_marked_non_exhaustive
     Checked [   0.007s] 75 checks; 73 passed, 1 failed, 1 warnings, 0 skipped

--- failure function_missing: pub fn removed or renamed ---

Description:
A publicly-visible function cannot be imported by its prior path. A `pub use` may have been removed, or the function itself may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.32.0/src/lints/function_missing.ron

Failed in:
  function pkg::function_missing, previously in file /home/m/dev/cargo-semver-checks/test_crates/manifest_tests/workspace_overrides/old/pkg/src/lib.rs:5

--- warning module_missing: pub module removed or renamed ---

Description:
A publicly-visible module cannot be imported by its prior path. A `pub use` may have been removed, or the module may have been renamed, removed, or made non-public.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.32.0/src/lints/module_missing.ron

Failed in:
  mod pkg::module_missing, previously in file /home/m/dev/cargo-semver-checks/test_crates/manifest_tests/workspace_overrides/old/pkg/src/lib.rs:9
     Summary semver requires new minor version: 0 major and 1 minor checks failed
     Warning produced 1 major and 0 minor level warnings
             produced warnings suggest new major version
    Finished [   0.673s] pkg

obi1kenobi · 2024-07-03T15:10:47Z

src/check_release.rs

        config
            .shell_print(
                "Checked",
                format_args!(
-                    "[{:>8.3}s] {} checks; {} passed, {} failed, {} unnecessary",
+                    "[{:>8.3}s] {} checks; {} passed, {} failed, {} warnings, {} skipped",


{} warnings seems a bit inconsistent here, because this is describing how many lints generated at least one warning, not how many warnings were generated.

warned looks more consistent but sounds a bit weird too.

What do you think about tweaking the whole thing like:

Suggested change

"[{:>8.3}s] {} checks; {} passed, {} failed, {} warnings, {} skipped",

"[{:>8.3}s] {} checks; {} pass, {} fail, {} warn, {} skip",

I'm on the fence about it personally, but leaning toward it.

I like it because it saves characters, but it does feel a little weird not being in past tense. It is definitely still clear and understandable, though, so I'm definitely open to changing it to this.

obi1kenobi · 2024-07-03T15:13:03Z

src/check_release.rs

@@ -396,6 +373,41 @@ pub(super) fn run_check_release(
                Color::Ansi(AnsiColor::Red),
                true,
            )?;
+        } else {


If errors or warnings got printed, let's make sure we leave an empty line between the last of those and the Summary line to improve readability. Otherwise the summary looks visually attached to the most recently printed error or warning block.

obi1kenobi · 2024-07-03T15:14:36Z

src/check_release.rs

@@ -407,7 +419,7 @@ pub(super) fn run_check_release(
            .shell_print(
                "Checked",
                format_args!(
-                    "[{:>8.3}s] {} checks; {} passed, {} unnecessary",
+                    "[{:>8.3}s] {} checks; {} passed, {} skipped",


If we end up making the passed -> pass / skipped -> skip change I suggested above, let's make sure we apply the corresponding change here as well for consistency.

Suggested change

"[{:>8.3}s] {} checks; {} passed, {} skipped",

"[{:>8.3}s] {} checks; {} pass, {} skip",

obi1kenobi · 2024-07-03T15:17:11Z

Honestly, this looks very good to me for a first pass. We can always polish everything more and we'll keep doing it in the future. No reason to make perfect be the enemy of good enough, let's keep moving forward.

- change failed -> fail, etc. in `Checked` summary - add a blank line before writing summary when fails/warns were printed - add summary line when all checks pass

obi1kenobi · 2024-07-03T20:23:06Z

src/check_release.rs

@@ -356,6 +356,7 @@ pub(super) fn run_check_release(
        let suggested_bump = suggested_versions.iter().max().copied();

        if let Some(required_bump) = required_bump {
+            writeln!(config.stderr())?;


Are you sure config.shell_print() goes to stderr, not stdout? We want the newline in the same stream.

It does.. Almost everything writes to stderr, except the --- failure/warning --- blocks. Does this newline belong to the Summary block or would it make more sense to write an extra newline after every --- failure/warning --- block?

I'm open to it either way, whatever is cleaner to do.

I think it makes more sense to put it with the summary block, as the --- failure --- blocks already separate themselves with newlines, so we would be adding two newlines in between each block.

Sounds good to me!

obi1kenobi · 2024-07-03T20:25:23Z

src/check_release.rs

@@ -374,6 +375,7 @@ pub(super) fn run_check_release(
                true,
            )?;
        } else {
+            writeln!(config.stderr())?;


This blank line appears to be printed if there were no errors, whether or not any warnings were produced. If no warnings were printed, I'm not sure we want a blank line there.

To me, conceptually, the blank line is the last line of each warning or error block, not a prefix to the summary line. So if there were no error or warning blocks (--- failure ...) then I probably wouldn't include a blank line here.

This is inside check_release.rs:299: if produced_errors || produced_warnings, so it must be true that if no errors are produced, some warnings are produced, although this isn't expressed well in the code. Would it make sense to change the else to an else if warnings_produced {...} else { unreachable!() } to be more expressive?

Yes, please! That would definitely have been more clear, and it's safer against future refactoring too.

src/check_release.rs

Co-authored-by: Predrag Gruevski <[email protected]>

obi1kenobi

I think this looks really good, very nicely done!

I have a couple of minor nitpicks and suggestions for reducing future maintainability cost, but they don't have to block this PR. I trust you to address them in a subsequent PR, so I'll merge this and we can move forward.

obi1kenobi · 2024-07-05T15:43:13Z

src/check_release.rs

+                    (true, _) => ("PASS", AnsiColor::Green),
+                    (false, LintLevel::Deny) => ("FAIL", AnsiColor::Red),
+                    (false, LintLevel::Warn) => ("WARN", AnsiColor::Yellow),
+                    (false, LintLevel::Allow) => unreachable!(),


Having seen and debugged a decent number of cases where an unreachable!() without any error message was hit, I've started to feel that it's worth spending an extra 10s to add more context to the crash (so it becomes more obvious and easily reproducible) than to spend many hours trying to reproduce a finicky error case triggered by unknown conditions.

Usually, in a situation where an unreachable statement got hit, the first change we'll make is add more context to the error so we understand what's going on. Adding that context up front doesn't meaningfully slow down development, but it makes for a much less frustrating debugging experience.

Suggested change

(false, LintLevel::Allow) => unreachable!(),

(false, LintLevel::Allow) => {

unreachable!(

"lint was unexpectedly executed at LintLevel::Allow: {semver_query:?}"

)

}

obi1kenobi · 2024-07-05T15:44:41Z

src/check_release.rs

+            match overrides.effective_lint_level(semver_query) {
+                LintLevel::Deny => results_with_errors.push((semver_query, results)),
+                LintLevel::Warn => results_with_warnings.push((semver_query, results)),
+                _ => (),


This is probably unreachable too, right?

Am I correct in thinking that the invariant is "LintLevel::Allow lints are skipped entirely, because there's no point in running them"?

obi1kenobi · 2024-07-05T15:47:49Z

src/check_release.rs

+                true,
+            )?;
+        } else {
+            unreachable!();


If there's a good description of what appears to have gone wrong for us to hit this, it would be great to put it here.

Even if right now it may seem obvious to you and me, we're still 200+ lines into a very complex function, and we might not think it's so obvious in 6 months.

suaviloquence added 8 commits June 26, 2024 11:36

skip queries that are configured less than warn

8291a59

- if we ever want an --ignore-warnings (or --ignore-less-than LEVEL) flag, we can change the hardcoded `LintLevel::Warn` to something else - should add a test to verify correct behavior

print lint level for each check in debug log line

3ed679a

- will be useful for tests without new API - the 12 chars is kind of arbitrary but 18 chars looked too wide

derive Ord on RequiredSemverUpdate

9257d75

- so we can call `.max()` on the `Vec` and get an `Option<update>` - it needs to be an `Option` now because there may be nothing (e.g., only warnings fail)

split query results into warnings and errors on failure

aafcc6c

change wording to be more consistent between warnings/errors

8474733

update workspace test

159a4db

use multiple asserts instead of .and() for better feedback on failure

1723de7

obi1kenobi reviewed Jun 26, 2024

View reviewed changes

suaviloquence and others added 4 commits June 27, 2024 20:15

apply suggestions from code review

194f094

Co-authored-by: Predrag Gruevski <[email protected]>

Merge branch 'main' into use-lint-level

a39a55a

Merge branch 'main' into use-lint-level

d518c49

reformat CLI and add new test

8efd986

- show PASS/FAIL/WARN in verbose mode - print `--- failure ---` or `--- warning ---` on triggered lint - update summary - add new test for when only warnings are produced

obi1kenobi reviewed Jul 3, 2024

View reviewed changes

suaviloquence added 2 commits July 3, 2024 12:50

clean up formatting

822439b

- change failed -> fail, etc. in `Checked` summary - add a blank line before writing summary when fails/warns were printed - add summary line when all checks pass

clippy fix

51023db

obi1kenobi reviewed Jul 3, 2024

View reviewed changes

suaviloquence commented Jul 3, 2024

View reviewed changes

src/check_release.rs Outdated Show resolved Hide resolved

suaviloquence and others added 3 commits July 3, 2024 20:39

Apply suggestions from code review

1a5ed08

Co-authored-by: Predrag Gruevski <[email protected]>

refactor else statement

f904be7

rename print_failed_lint -> print_triggered_lint

29fa917

obi1kenobi approved these changes Jul 5, 2024

View reviewed changes

obi1kenobi merged commit fe1830f into obi1kenobi:main Jul 5, 2024
35 checks passed

suaviloquence mentioned this pull request Jul 5, 2024

annotate unreachable!() statements #808

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle lint level in check_release #805

Handle lint level in check_release #805

suaviloquence commented Jun 26, 2024 •

edited

Loading

obi1kenobi left a comment

obi1kenobi Jun 26, 2024

obi1kenobi Jun 26, 2024

obi1kenobi Jun 26, 2024

obi1kenobi Jun 26, 2024

obi1kenobi Jun 26, 2024

obi1kenobi Jun 26, 2024

obi1kenobi commented Jun 26, 2024

suaviloquence commented Jun 27, 2024

suaviloquence commented Jul 2, 2024

obi1kenobi Jul 3, 2024 •

edited

Loading

suaviloquence Jul 3, 2024

obi1kenobi Jul 3, 2024

obi1kenobi Jul 3, 2024

obi1kenobi commented Jul 3, 2024

obi1kenobi Jul 3, 2024

suaviloquence Jul 3, 2024

obi1kenobi Jul 3, 2024

suaviloquence Jul 3, 2024

obi1kenobi Jul 4, 2024

obi1kenobi Jul 3, 2024

suaviloquence Jul 3, 2024

obi1kenobi Jul 3, 2024

obi1kenobi left a comment

obi1kenobi Jul 5, 2024

obi1kenobi Jul 5, 2024

obi1kenobi Jul 5, 2024

	"[{:>8.3}s] {} checks; {} passed, {} failed, {} warnings, {} skipped",
	"[{:>8.3}s] {} checks; {} pass, {} fail, {} warn, {} skip",

	"[{:>8.3}s] {} checks; {} passed, {} skipped",
	"[{:>8.3}s] {} checks; {} pass, {} skip",

Handle lint level in check_release #805

Handle lint level in check_release #805

Conversation

suaviloquence commented Jun 26, 2024 • edited Loading

obi1kenobi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obi1kenobi commented Jun 26, 2024

suaviloquence commented Jun 27, 2024

suaviloquence commented Jul 2, 2024

obi1kenobi Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obi1kenobi commented Jul 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obi1kenobi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suaviloquence commented Jun 26, 2024 •

edited

Loading

obi1kenobi Jul 3, 2024 •

edited

Loading