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

[New] Show device location with NMEA data sources #144

Merged
merged 56 commits into from
Oct 30, 2023
Merged

Conversation

mhdostal
Copy link
Member

@mhdostal mhdostal commented Mar 27, 2023

Description

This PR implements Show device location with NMEA data sources in Maps category.
URL to README: here

Linked Issue(s)

  • swift/issues/2258

To Discuss

I've taken this over from Mark since he is occupied with feature team work. I made the following changes since 5ae8e26

  1. Instead of pushing the data in the mock data object, I made the object to provide an asyncstream that generate the mock data, to better simulate a real world usecase. Discussion at [New] Show device location with NMEA data sources #144 (comment)
  2. I didn't touch the Task.detached part created by Mark. Initially I thought it would be better to use a task group for this purpose, but the tasks seem to work well, so I didn't change them other than making them private. Discussion at [New] Show device location with NMEA data sources #144 (comment)
  3. I didn't move the "status text" and their formatting methods to the view. While it is a better practice to only localize and format strings at the view level, it makes the code harder to read without adding too much benefit. My reasons were explained here: [New] Run valve isolation trace #203 (comment)
  4. Please feel free to comment in the remaining open conversations.

Screenshots

To do before merge

@mhdostal mhdostal changed the base branch from main to v.next April 11, 2023 19:15
@mhdostal mhdostal marked this pull request as ready for review April 11, 2023 20:58
@mhdostal mhdostal requested a review from yo1995 April 11, 2023 20:58
Copy link
Collaborator

@yo1995 yo1995 left a comment

Choose a reason for hiding this comment

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

Please also merge from the latest v.next. Here are some initial thoughts. After they are addressed, I'll add someone who's more familiar with async tasks to review.

@yo1995
Copy link
Collaborator

yo1995 commented Sep 8, 2023

I'd like to hear more feedback on the detached task part of code. See the unresolved conversations.

All other suggestions are applied.

…DME.md

Co-authored-by: Destiny Hochhalter <117859673+des12437@users.noreply.github.com>
des12437
des12437 previously approved these changes Sep 11, 2023
yo1995 and others added 4 commits October 30, 2023 14:18
private(set)

Co-authored-by: Destiny Hochhalter <117859673+des12437@users.noreply.github.com>
@yo1995
Copy link
Collaborator

yo1995 commented Oct 30, 2023

Thank everyone for the efforts to put this one together! 🥳

@yo1995 yo1995 merged commit 75449ff into v.next Oct 30, 2023
1 check passed
@yo1995 yo1995 deleted the mhd/NMEADataSources branch October 30, 2023 23:27
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

4 participants