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

ci(rust): check MSRV in CI and fix clippy #3054

Merged
merged 8 commits into from
Nov 1, 2024

Conversation

wjones127
Copy link
Contributor

No description provided.

@github-actions github-actions bot added the ci Github Action or Test issues label Oct 28, 2024
Copy link

ACTION NEEDED
Lance follows the Conventional Commits specification for release automation.

The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification.

For details on the error please inspect the "PR Title Check" action.

@wjones127 wjones127 changed the title ci: Check MSRV in CI ci(rust): check MSRV in CI Oct 28, 2024
@wjones127
Copy link
Contributor Author

I'm struggling to understand what's going on with clippy. I get errors when I run it locally, even with the same version of clippy:

❯ cargo clippy --version
clippy 0.1.81 (eeb90cda 2024-09-04)
❯ cargo clippy --features cli,dynamodb,tensorflow,dynamodb_tests --tests --benches -- -D warnings
    Checking lance-arrow v0.19.2 (/Users/willjones/Documents/lance/rust/lance-arrow)
   Compiling lance-encoding v0.19.2 (/Users/willjones/Documents/lance/rust/lance-encoding)
   ...
    Checking lance-io v0.19.2 (/Users/willjones/Documents/lance/rust/lance-io)
    Checking lance-datafusion v0.19.2 (/Users/willjones/Documents/lance/rust/lance-datafusion)
error: this could be rewritten as `let...else`
   --> rust/lance-index/src/scalar/inverted/builder.rs:455:13
    |
455 | /             let token = if let Some(token) = token {
456 | |                 token
457 | |             } else {
458 | |                 continue;
459 | |             };
    | |______________^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#manual_let_else
    = note: `-D clippy::manual-let-else` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(clippy::manual_let_else)]`
help: consider writing
    |
455 ~             let Some(token) = token else {
456 +                 continue;
457 +             };
    |

error: pub(crate) struct inside private module
   --> rust/lance-index/src/scalar/inverted/builder.rs:564:1
    |
564 | pub(crate) struct PostingReader {
    | ----------^^^^^^^^^^^^^^^^^^^^^
    | |
    | help: consider using: `pub`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#redundant_pub_crate
    = note: `-D clippy::redundant-pub-crate` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(clippy::redundant_pub_crate)]`

error: pub(crate) function inside private module
   --> rust/lance-index/src/scalar/inverted/builder.rs:708:1
    |
708 | pub(crate) fn inverted_list_schema(with_position: bool) -> SchemaRef {
    | ----------^^^^^^^^^^^^^^^^^^^^^^^^
    | |
    | help: consider using: `pub`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#redundant_pub_crate

error: unnecessary structure name repetition
  --> rust/lance-index/src/scalar/inverted/tokenizer.rs:49:9
   |
49 |         TokenizerConfig {
   |         ^^^^^^^^^^^^^^^ help: use the applicable keyword: `Self`
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#use_self
   = note: `-D clippy::use-self` implied by `-D warnings`
   = help: to override `-D warnings` add `#[allow(clippy::use_self)]`

error: could not compile `lance-index` (lib) due to 4 previous errors
warning: build failed, waiting for other jobs to finish...
error: could not compile `lance-index` (lib test) due to 4 previous errors

@wjones127 wjones127 changed the title ci(rust): check MSRV in CI ci(rust): check MSRV in CI and fix clippy Oct 30, 2024
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For whatever reason, these flags weren't being picked up in CI. So I've moved them to Cargo.toml, which seems to work.

@wjones127 wjones127 marked this pull request as ready for review October 30, 2024 01:55
@codecov-commenter
Copy link

codecov-commenter commented Oct 30, 2024

Codecov Report

Attention: Patch coverage is 86.79245% with 14 lines in your changes missing coverage. Please review.

Project coverage is 77.54%. Comparing base (66c5f2c) to head (7464e19).

Files with missing lines Patch % Lines
rust/lance/src/dataset/transaction.rs 72.72% 3 Missing ⚠️
rust/lance-datafusion/src/exec.rs 33.33% 2 Missing ⚠️
rust/lance-arrow/src/deepcopy.rs 0.00% 1 Missing ⚠️
rust/lance-datafusion/src/planner.rs 80.00% 0 Missing and 1 partial ⚠️
rust/lance-encoding-datafusion/src/lib.rs 0.00% 1 Missing ⚠️
rust/lance-encoding/src/decoder.rs 75.00% 1 Missing ⚠️
rust/lance-io/src/object_store.rs 0.00% 0 Missing and 1 partial ⚠️
rust/lance/src/dataset/write/merge_insert.rs 50.00% 1 Missing ⚠️
rust/lance/src/dataset/write/update.rs 66.66% 0 Missing and 1 partial ⚠️
rust/lance/src/index/vector/pq.rs 66.66% 1 Missing ⚠️
... and 1 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3054      +/-   ##
==========================================
- Coverage   77.61%   77.54%   -0.08%     
==========================================
  Files         240      240              
  Lines       78683    78573     -110     
  Branches    78683    78573     -110     
==========================================
- Hits        61073    60931     -142     
- Misses      14495    14504       +9     
- Partials     3115     3138      +23     
Flag Coverage Δ
unittests 77.54% <86.79%> (-0.08%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@westonpace westonpace left a comment

Choose a reason for hiding this comment

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

Thanks for digging in and figuring this out. It was annoying me as well :)

@wjones127 wjones127 merged commit 638eaf4 into lancedb:main Nov 1, 2024
24 checks passed
@wjones127 wjones127 deleted the ci/check-msrv branch November 1, 2024 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci Github Action or Test issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants