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

Globe default value #13

Merged
merged 1 commit into from Jul 18, 2014
Merged

Globe default value #13

merged 1 commit into from Jul 18, 2014

Conversation

thiemowmde
Copy link
Contributor

The $globe parameter of the constructor always had a default value, right? What I'm doing here is just reimplementing the exact same semantics in a slightly different way, right?

Before: new GlobeCoordinateValue( $latLang, $precision ) was possible but new GlobeCoordinateValue( $latLang, $precision, null ) failed. Doesn't make much sense if you think about, especially in terms of deserialization.

After: Both calls use the default value.

Bug: 66632

@JeroenDeDauw
Copy link
Member

This will not fully fix bug 66632

@thiemowmde
Copy link
Contributor Author

@JeroenDeDauw Can you explain? My impression was that it indeed solves the problem. It should replace NULL values in the database with the default value.

@JeroenDeDauw
Copy link
Member

Values that could no longer be dezerlaized used to be turned into an UnDeserializableValue. We forgot to implement that in the new deserialization code. This PR does not address that issue, though it might itself be valid as well, I don't know.

@thiemowmde
Copy link
Contributor Author

it might itself be valid as well

I think so. Earth is the default. I think there is no question about this. If NULL is stored (for whatever reason) why should it not fall back to the default?

@JeroenDeDauw
Copy link
Member

To be clear: I have no objections against this commit and did not really review it. @filbertkm probably has an opinion on the actual change

@JeroenDeDauw
Copy link
Member

Needs rebase

@brightbyte
Copy link

+1 looks fine. doesn't travis run here?...

Before: new GlobeCoordinateValue( $latLang, $precision ) was possible but
new GlobeCoordinateValue( $latLang, $precision, null ) failed. Doesn't make
much sense if you think about, especially in terms of deserialization.

After: Both calls use the default value.

Bug: 66632
@JeroenDeDauw
Copy link
Member

Ok, I will then merge, and make sure the next rel is 0.3 (or 1.0)

JeroenDeDauw added a commit that referenced this pull request Jul 18, 2014
@JeroenDeDauw JeroenDeDauw merged commit 48b3f5d into master Jul 18, 2014
@JeroenDeDauw JeroenDeDauw deleted the globe-null branch July 18, 2014 16:09
@JeroenDeDauw
Copy link
Member

@thiemowmde can you up the rel notes?

@thiemowmde
Copy link
Contributor Author

Done. Fully agree to what @filbertkm said.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants