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

Extend CLLocationCoordinate2D with isValid computed property #102

Merged
merged 7 commits into from
Jan 19, 2022

Conversation

andrew-winn-br
Copy link
Contributor

Attempting to use an invalid coordinate will crash the app. This provides a convenient computed property to check if a CLLocationCoordinate2D is valid or not.

Copy link
Contributor

@rmirabelli rmirabelli left a comment

Choose a reason for hiding this comment

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

I'm pretty sure the import should be CoreLocation.

andrew-winn-br and others added 2 commits August 25, 2021 15:27
Co-authored-by: Earl Gaspard <83370606+br-earl-gaspard@users.noreply.github.com>
Incorporate PR feedback
Change import
@br-earl-gaspard
Copy link
Contributor

It would be great to get some unit tests added for this.

Copy link
Contributor

@br-tyler-milner br-tyler-milner left a comment

Choose a reason for hiding this comment

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

Looks good. Just needs an entry added in CHANGELOG.md.

@andrew-winn-br
Copy link
Contributor Author

@br-tyler-milner

Looks good. Just needs an entry added in CHANGELOG.md.

Should it be a new version number?

@br-tyler-milner
Copy link
Contributor

@andrew-winn-br just an entry under the "Enhancements" section. We'll create a version number entry when we do the next release of Utilikit. See this change on a previous PR for an example.

Add tests
Update Change log
wmcginty
wmcginty previously approved these changes Aug 27, 2021
longitude: CLLocationDegrees(-180))
]

XCTAssertNil(coordinates.first { !$0.isValid })
Copy link
Contributor

Choose a reason for hiding this comment

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

This may just be personal preference, but to me this would be more clear as XCTAssertTrue(coordinates.allSatisfy { $0.isValid })

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. I can change it and the invalid coordinates test.

Clarify tests.
wmcginty
wmcginty previously approved these changes Aug 27, 2021
@br-tyler-milner br-tyler-milner dismissed stale reviews from br-earl-gaspard, wmcginty, and themself via 6176de8 January 14, 2022 20:59
@br-tyler-milner br-tyler-milner requested a review from a team as a code owner January 14, 2022 20:59
@br-tyler-milner
Copy link
Contributor

@rmirabelli looks like this just needs your thumbs up for us to go ahead and merge.

@wmcginty
Copy link
Contributor

@andrew-winn-br I think in addition to Russell's re-approval we'll want to merge in the changes from main to get the updated CI running over the PR.

@wmcginty
Copy link
Contributor

You know what, looking at it - it's a small enough change that I'll just deal with any weirdness on the release branch :). Merging.

@wmcginty wmcginty merged commit 601fbe1 into BottleRocketStudios:main Jan 19, 2022
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.

None yet

5 participants