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

[BUG] Remove enum imports from match statements #3436

Merged
merged 2 commits into from
Nov 30, 2024
Merged

Conversation

raunakab
Copy link
Contributor

@raunakab raunakab commented Nov 26, 2024

Overview

Importing enums is extremely bad style in Rust. This can lead to nasty errors in the future when enum definitions change.

Please stop doing this.

@github-actions github-actions bot added the bug Something isn't working label Nov 26, 2024
@raunakab raunakab marked this pull request as ready for review November 26, 2024 22:02
@andrewgazelka
Copy link
Contributor

hooray I agree 🕺

@raunakab
Copy link
Contributor Author

raunakab commented Nov 26, 2024

hooray I agree 🕺

@andrewgazelka This is a large problem in our codebase. I will (in the future) start merging in small, piecemeal PRs to address this issue.

Copy link

codspeed-hq bot commented Nov 26, 2024

CodSpeed Performance Report

Merging #3436 will improve performances by ×2.1

Comparing bug/remove-enum-imports (0ab08e4) with main (3ece13a)

Summary

⚡ 1 improvements
✅ 16 untouched benchmarks

Benchmarks breakdown

Benchmark main bug/remove-enum-imports Change
test_show[100 Small Files] 31.9 ms 14.9 ms ×2.1

@andrewgazelka
Copy link
Contributor

andrewgazelka commented Nov 26, 2024

hooray I agree 🕺

@andrewgazelka This is a large problem in our codebase. I'm going to start merging in small, piecemeal PRs to address this issue.

Do we want to prio this though?

BigDaft

ideally if we want to enforce something should also add to clippy lints
https://rust-lang.github.io/rust-clippy/master/index.html

@raunakab
Copy link
Contributor Author

hooray I agree 🕺

@andrewgazelka This is a large problem in our codebase. I'm going to start merging in small, piecemeal PRs to address this issue.

Do we want to prio this though?

BigDaft

ideally if we want to enforce something should also add to clippy lints https://rust-lang.github.io/rust-clippy/master/index.html

Probably not right now, but in the future.

@andrewgazelka andrewgazelka self-requested a review November 26, 2024 22:08
Copy link

codecov bot commented Nov 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.35%. Comparing base (3ece13a) to head (0ab08e4).
Report is 13 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #3436   +/-   ##
=======================================
  Coverage   77.35%   77.35%           
=======================================
  Files         684      684           
  Lines       83637    83637           
=======================================
  Hits        64694    64694           
  Misses      18943    18943           

see 1 file with indirect coverage changes

@raunakab raunakab merged commit a16a045 into main Nov 30, 2024
46 checks passed
@raunakab raunakab deleted the bug/remove-enum-imports branch November 30, 2024 00:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants