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

refactor: replace LineOrPoint struct with enum #1047

Merged
merged 2 commits into from
Aug 25, 2023

Conversation

RobWalt
Copy link
Contributor

@RobWalt RobWalt commented Aug 1, 2023

  • I agree to follow the project's code of conduct.
  • I added an entry to CHANGES.md if knowledge of this change could be valuable to users.
  • Tests still pass

This small PR swaps out the LineOrPoint struct with an enum approach. It doesn't change the functionality of the type but the use of it should be much more readable now.

@rmanoka pinging you since it is mainly your code and I would appreciate a review if you find the time. I'm trying to make improvements step by step but I need to understand the code better (hence this PR). Are PRs like this ok for you?

Copy link
Member

@michaelkirk michaelkirk left a comment

Choose a reason for hiding this comment

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

I don't see any errors in the translation, and it "feels" nicer, but I defer to @rmanoka about wether we want to merge a refactor like this to the sweep algo.

@rmanoka
Copy link
Contributor

rmanoka commented Aug 2, 2023

lgtm too; can you fix the lint and fuzz CI errors?

@RobWalt
Copy link
Contributor Author

RobWalt commented Aug 2, 2023

Just looked into it. The error is

 --> geo/src/algorithm/sweep/line_or_point.rs:128:32
    |
128 |         T::Ker::orient2d(*self.left, *self.right, other)
    |                                ^^^^ method, not a field
    |

I just double checked and my code doesn't include this anymore. Could it be related to some kind of cache or dependency from geo to itself?

Comment on lines -165 to +198
T::Ker::orient2d(*p1, *q1, *p2)
T::Ker::orient2d(**left_a, **right_a, **left_b)
.as_ordering()
.then_with(|| T::Ker::orient2d(*p1, *q1, *q2).as_ordering()),
.then_with(|| {
T::Ker::orient2d(**left_a, **right_a, **right_b).as_ordering()
}),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here are my changes, the error isn't even showing the code from the previous version 👀 I rg-d over the whole repo and I couldn't find the code that was mentioned in the error message ...

@RobWalt RobWalt force-pushed the refactor/lineorpoint-to-enum branch from 8d20cf0 to 7bc35c0 Compare August 2, 2023 06:39
@michaelkirk
Copy link
Member

michaelkirk commented Aug 7, 2023

CI updates your branch locally before running the tests to make sure they are compatible with what's in the main branch. Looks like the code in question was merged since your branch was created.

Can you either rebase and force push or merge main into your branch and address the issues?

@RobWalt RobWalt force-pushed the refactor/lineorpoint-to-enum branch from 7bc35c0 to 83b3f34 Compare August 18, 2023 07:52
@RobWalt
Copy link
Contributor Author

RobWalt commented Aug 18, 2023

Done 👍

@michaelkirk michaelkirk added this pull request to the merge queue Aug 25, 2023
Merged via the queue into georust:main with commit c4ee486 Aug 25, 2023
13 checks passed
@RobWalt RobWalt deleted the refactor/lineorpoint-to-enum branch August 26, 2023 08:36
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.

4 participants