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

Clippy: Remove .clone() where possible. #665

Merged

Conversation

martinfrances107
Copy link
Contributor


Looking at the output of cargo clippy

I want to cherry pick the single aspect I find most troublesome.
In general it is better to use copy over clone.
It has been fixed in a couple of places.

martinfrances107 and others added 2 commits October 12, 2021 16:39
Improved readability.

Co-authored-by: Laurențiu Nicola <[email protected]>
simpler to use  self.polygon()

Co-authored-by: Laurențiu Nicola <[email protected]>
@lnicola
Copy link
Member

lnicola commented Oct 12, 2021

Thanks!

bors r+

bors bot added a commit that referenced this pull request Oct 12, 2021
665: Clippy: Remove .clone() where possible. r=lnicola a=martinfrances107

- [x] I agree to follow the project's [code of conduct](https://github.com/georust/geo/blob/master/CODE_OF_CONDUCT.md).
---

Looking at the output of `cargo clippy`

I want to cherry pick the single aspect I find most troublesome. 
In general it is better to use copy over clone.
It has been fixed in a couple of places.


Co-authored-by: Martin <[email protected]>
Co-authored-by: martin frances <[email protected]>
@bors
Copy link
Contributor

bors bot commented Oct 12, 2021

Build failed:

@lnicola
Copy link
Member

lnicola commented Oct 12, 2021

CI errors looks like a precision issue or an over-zealous check.

@martinfrances107
Copy link
Contributor Author

looking at the test ouput - it is a precision issue

---- algorithm::map_coords::test::test_fallible_proj stdout ----
thread 'algorithm::map_coords::test::test_fallible_proj' panicked at 'assertion failed: (left == right)
left: 3497301.5918027186,
right: 3497301.5918027232', geo/src/algorithm/map_coords.rs:837:9

so the last assert_eq!() in test_failing_proj

fn test_fallible_proj() {
    use proj::Proj;
    let from = "EPSG:4326";
    let to = "EPSG:2230";
    let to_feet = Proj::new_known_crs(&from, &to, None).unwrap();

    let f = |x: f64, y: f64| {
        let shifted = to_feet.convert((x, y))?;
        Ok((shifted.x(), shifted.y()))
    };
    // 👽
    let usa_m = Point::new(-115.797615, 37.2647978);
    let usa_ft = usa_m.try_map_coords(|&(x, y)| f(x, y)).unwrap();
    assert_eq!(6693625.67217475, usa_ft.x());
    assert_eq!(3497301.5918027186, usa_ft.y());
}

Comparing floating point numbers needs care and attention, here is the issue which solved the issue brittle

The solution is to use assert_relative_eq!() where possible .. this work has already been done .. we just need to uniformly apply the policy

just as an example multipolygon_two_polygons_test() does the correct thing.

    fn multipolygon_two_polygons_test() {
        let linestring =
            LineString::from(vec![p(2., 1.), p(5., 1.), p(5., 3.), p(2., 3.), p(2., 1.)]);
        let poly1 = Polygon::new(linestring, Vec::new());
        let linestring =
            LineString::from(vec![p(7., 1.), p(8., 1.), p(8., 2.), p(7., 2.), p(7., 1.)]);
        let poly2 = Polygon::new(linestring, Vec::new());
        let centroid = MultiPolygon(vec![poly1, poly2]).centroid().unwrap();
        assert_relative_eq!(
            centroid,
            point![x: 4.071428571428571, y: 1.9285714285714286]
        );
    }

I am going to create an issue to solve this today.. I will post an update here with a link.

@lnicola
Copy link
Member

lnicola commented Oct 13, 2021

Yeah, assert_relative_eq is already used in other places. I think it was an oversight.

@martinfrances107
Copy link
Contributor Author

Ok here is the new issue... I will create a patch later today

#666

@michaelkirk
Copy link
Member

bors retry

@bors
Copy link
Contributor

bors bot commented Oct 13, 2021

Build succeeded:

@bors bors bot merged commit f43af7a into georust:master Oct 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants