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

parse new <location/> metadata value #117

Merged
merged 3 commits into from Jul 17, 2014
Merged

parse new <location/> metadata value #117

merged 3 commits into from Jul 17, 2014

Conversation

dannyroberts
Copy link
Member

No description provided.

@dannyroberts
Copy link
Member Author

@czue tests pass, but should review carefully for correctness and/or things you think I should add a test for

@czue
Copy link
Member

czue commented Jul 12, 2014

@dannyroberts no longer mergeable (sorry been delinquent)

@czue
Copy link
Member

czue commented Jul 12, 2014

looks good though. curious whether you considered making a GeoPointProperty type of thing to hold the data versus keeping as a string (either way would be easyish to roll in later so am good with merging this once it's mergeable)

@dannyroberts
Copy link
Member Author

Yeah, I don't have a clear philosophical or practical argument for or against doing something like GeoPointProperty. I guess the benefits are validation and ease of access to the data, so it seems like a positive. A danger incorrect validation code that crashes forms and prevents them from going through (like if we require something stricter than the full range of what the phone might send), but that's a short term thing that can be hotfixed as it comes, probably without irrecoverable damage, due to where it 500s.

@dannyroberts
Copy link
Member Author

How's that?

@czue
Copy link
Member

czue commented Jul 14, 2014

@czue
Copy link
Member

czue commented Jul 14, 2014

(and think you've correctly identified the tradeoffs and am happy with this outcome though would have been fine with either)

@dannyroberts
Copy link
Member Author

czue added a commit that referenced this pull request Jul 17, 2014
parse new <location/> metadata value
@czue czue merged commit de33603 into master Jul 17, 2014
@czue czue deleted the gps branch July 17, 2014 14:43
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

2 participants