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

Add new lint manual_duration_calcs #6490

Closed
wants to merge 37 commits into from
Closed

Conversation

himanoa
Copy link

@himanoa himanoa commented Dec 21, 2020

fixes #6068

  • Followed lint naming conventions
  • Added passing UI tests (including committed .stderr file)
  • cargo test passes locally
  • Executed cargo dev update_lints
  • Added lint documentation
  • Run cargo dev fmt

Note that you can skip the above if you are just opening a WIP PR in
order to get feedback.

Delete this line and everything above before opening your PR.


changelog: Add new lint manual_duration_calcs

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @flip1995 (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Dec 21, 2020
@himanoa himanoa marked this pull request as ready for review December 21, 2020 16:55
@himanoa himanoa marked this pull request as draft December 21, 2020 17:20
Rename from manual_re_implementation_as_secs_f64_for_mul to manual_re_implementation_lower_the_unit
Aligh arguments order with other functions.
@giraffate
Copy link
Contributor

It seems to fail dogfood test.

@himanoa
Copy link
Author

himanoa commented Dec 24, 2020

I'm doing refactoring now to do pass the dogfood test.

Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

Yes, please do some refactoring. Splitting things up in functions will help a lot.

{
if_chain! {
if let Some((mul_ident, mul_receiver, multiplier, plus_ident, plus_receiver, plus_cast_type)) =
match (&left.kind, &right.kind) {
Copy link
Member

Choose a reason for hiding this comment

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

Please refactor this out in an extra function.

}

#[derive(Debug)]
enum Number {
Copy link
Member

Choose a reason for hiding this comment

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

Also this. enums in fn in impls is a bit too deep IMO.

clippy_lints/src/lib.rs Show resolved Hide resolved
@flip1995 flip1995 added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Dec 28, 2020
error: no manual re-implementationa of the as_millis
--> $DIR/manual_duration_calcs.rs:33:23
|
LL | let _secs_f64_1 = (dur.as_secs() as f64 * 1_000.0 + dur.subsec_millis() as f64) / 1000.0;
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems to fail tests at this line and next line. I haven't checked that in detail, but the lint is emitted twice here and rustfix seems to fail.

Copy link
Author

@himanoa himanoa Jan 5, 2021

Choose a reason for hiding this comment

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

I know rustfix conflicting between manual_re_implementation_lower_the_unit and duration_as_secs when this pattern.

let _secs_f64_1 = (dur.as_secs() as f64 * 1_000.0 + dur.subsec_millis() as f64) / 1000.0
//                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ manual_re_implementation_lower_the_unit 
//                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ duration_as_secs

I am in trouble at can't resolve this conflict.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO, how about checking if parent expression in manual_re_implementation_lower_the_unit sees duration_as_secs?

@flip1995
Copy link
Member

Those are a huge amount of changes to the code, introducing multiple functions. It would take me a day to understand what every function/struct does (i.e. what pattern it matches). Please add more (doc) comments explaining the functions and structs introduced in this PR.

@bors
Copy link
Collaborator

bors commented Jan 22, 2021

☔ The latest upstream changes (presumably #6591) made this pull request unmergeable. Please resolve the merge conflicts.

@giraffate
Copy link
Contributor

ping from triage @himanoa. Can you add these?
#6490 (comment)

Please add more (doc) comments explaining the functions and structs introduced in this PR.

@giraffate
Copy link
Contributor

ping from triage @himanoa. According to the triage procedure, I'm closing this because 2 weeks have passed with no activity. If you have more time to work on this, feel free to reopen this.

@giraffate giraffate closed this Feb 22, 2021
@giraffate giraffate added S-inactive-closed Status: Closed due to inactivity and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Feb 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-inactive-closed Status: Closed due to inactivity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Manual Duration::{as_nanos,as_milis,as_secs_f64,as_secs_f32} implementation
5 participants