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

Adding Latitude and Longitude to Twitter Status Update... #714

Merged
merged 6 commits into from Dec 15, 2016
Merged

Adding Latitude and Longitude to Twitter Status Update... #714

merged 6 commits into from Dec 15, 2016

Conversation

@SpilledMilkCOM
Copy link
Contributor

@SpilledMilkCOM SpilledMilkCOM commented Dec 13, 2016

This is my first "official" PR so I HOPE I'm following the rules. I REALLY would like the Lat/Long added to the status. This looks like it's working through your test application. I did not see any Unit Tests for Twitter otherwise I would have added some. In the account that I tested with I only see "Colorado, USA" versus the lat/long (which I think used to be displayed last year or so). That might be a setting on a per account basis. As I was doing my research and testing, it didn't take much to add the REST of the UPDATE parameters through the mechanism I chose. It also lends itself for extension as well without changing the provider. I created a NEW method so I would not break the API, and then under the covers call the new API from the old API. I hope this is all I need to do so I can continue with my UWP application that used to use TweetSharp in the old Windows Phone 8.1 world. THANKS!! I hope this helps! Thank YOU for all of the other plumbing and reducing my work. So I thought I would give back a few hours...

Parker Smart added 2 commits Dec 13, 2016
… as to NOT break the existing API. Also updated the sample app for testing purposes.
@dnfclas
Copy link

@dnfclas dnfclas commented Dec 13, 2016

Hi @SpilledMilkCOM, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla2.dotnetfoundation.org.

TTYL, DNFBOT;

@dnfclas
Copy link

@dnfclas dnfclas commented Dec 13, 2016

@SpilledMilkCOM, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, DNFBOT;

Copy link
Contributor

@deltakosh deltakosh left a comment

This is what I can call an almost perfect PR :) I would just ask for a documentation update to mention the great features you add! (And perhaps add a new line in the .bind to show users they can tweet a text OR a status)

@deltakosh
Copy link
Contributor

@deltakosh deltakosh commented Dec 13, 2016

A cool option could be to automatically (under an option) get the device coordinates :)

@SpilledMilkCOM
Copy link
Contributor Author

@SpilledMilkCOM SpilledMilkCOM commented Dec 14, 2016

I'll have to dig around and see where I can update the documentation... Unless you want to let me know where that is here. I wasn't sure how to do that. I seriously WAS going to add the Geolocation, BUT you have to adjust the project manifest file to include that capability which I didn't figure anybody would WANT me to do. I will DEFINITELY DO that though. I already have the code to get the lat/long so I will add it to the sample application. I don't think that the Twitter namespace should rely on the Geolocation stuff since it should be straight Twitter, but adding it to the sample application would show people how they can do that easily. Since you want to display this lat/long anyway in your UI, you would be getting that value externally versus grabbing it from a Twitter toolkit class. I'll whip something up so you can see it.

@SpilledMilkCOM
Copy link
Contributor Author

@SpilledMilkCOM SpilledMilkCOM commented Dec 14, 2016

Looks like the PR updated automagically. Let me know if there are problems. Hopefully this makes it into the official release SOON! I'd LOVE to get this out with my latest UWP converted app.

I apologize for not "discussing" this in the forums first, but I needed the lat/long stuff and I figured other people would use it too. (Plus I didn't read the official documentation until I found the folder and started to browse it.) THEN I just figured I'd add the rest of the params since the TwitterStatus class is so easy. I'm glad somebody ELSE did the media. :) That was a pain using TweetSharp, but doable.

@deltakosh
Copy link
Contributor

@deltakosh deltakosh commented Dec 14, 2016

No need for apologize :) this is really good stuff
I'ok check and merge asap:)

@deltakosh
Copy link
Contributor

@deltakosh deltakosh commented Dec 14, 2016

Btw I did the media and it was painful:)
As soon as we will merge you will be able to use the prerelease repo to get a nuget package for your app. Hopefully tomorrow

@SpilledMilkCOM
Copy link
Contributor Author

@SpilledMilkCOM SpilledMilkCOM commented Dec 14, 2016

THANK YOU!! I'll play with the sample app over my vacation and see what I can help with...

@deltakosh
Copy link
Contributor

@deltakosh deltakosh commented Dec 14, 2016

Perfect.Just waiting for a second reviewer and we can merge!

{
var geolocator = new Geolocator();

var position = await geolocator.GetGeopositionAsync();
Copy link
Contributor

@dotMorten dotMorten Dec 14, 2016

This could fail. Try/catch ?

Copy link
Contributor Author

@SpilledMilkCOM SpilledMilkCOM Dec 15, 2016

No problem. I'll a try/catch similar to the ones in the file.

/// NOTE: This parameter will be ignored unless it is inside the range -90.0 to +90.0 (North is positive) inclusive.
/// It will also be ignored if there isn’t a corresponding long parameter.
/// </summary>
public decimal? Latitude { get; set; }
Copy link
Contributor

@dotMorten dotMorten Dec 14, 2016

Why decimal? double should be sufficient

Copy link
Contributor Author

@SpilledMilkCOM SpilledMilkCOM Dec 15, 2016

I'm just used to decimal since I write mostly financial software. I can change it.

/// This parameter will be ignored if outside that range, if it is not a number, if geo_enabled is disabled,
/// or if there not a corresponding lat parameter.
/// </summary>
public decimal? Longitude { get; set; }
Copy link
Contributor

@dotMorten dotMorten Dec 14, 2016

'double' ?

Copy link
Contributor Author

@SpilledMilkCOM SpilledMilkCOM Dec 15, 2016

I'm just used to decimal since I write mostly financial software. I can change it.

public string PlaceId { get; set; }

/// <summary>
/// Gets or sets a value indicating whether the Tweet contains sensitive content (suchas nudity, etc.).
Copy link
Contributor

@dotMorten dotMorten Dec 14, 2016

typo "suchas"

Copy link
Contributor Author

@SpilledMilkCOM SpilledMilkCOM Dec 15, 2016

Oops.

result = $"{result}&lat={Latitude.Value}&long={Longitude.Value}";
}

result = AddRequestParameter(result, "display_coordinates", DisplayCoordinates);
Copy link
Contributor

@dotMorten dotMorten Dec 14, 2016

Should this only be added if there's latitude and longitude values?

Copy link
Contributor Author

@SpilledMilkCOM SpilledMilkCOM Dec 15, 2016

It would get ignored, but no need to send them if there's no lat/long. I'll fix that.


if (Latitude.HasValue && Longitude.HasValue)
{
result = $"{result}&lat={Latitude.Value}&long={Longitude.Value}";
Copy link
Contributor

@dotMorten dotMorten Dec 14, 2016

double-to-string must specify CultureInfo.Invariant or it'll use the wrong comma separator and prefix-location in some cultures

Copy link
Contributor Author

@SpilledMilkCOM SpilledMilkCOM Dec 15, 2016

Ah yes. Twitter requires the period "." decimal point versus the comma ",". I'll change it...


if (value)
{
result = $"{result}&{parameterName}=true";
Copy link
Contributor

@dotMorten dotMorten Dec 14, 2016

Why only true? What if the default is true and you want to force it to false?

Copy link
Contributor Author

@SpilledMilkCOM SpilledMilkCOM Dec 15, 2016

I don't believe any of their settings are defaulted to be true. Twitter Update Reference They've named their Boolean parameters in such a way that setting them to true turns the feature on. By omitting them they will stay false. I suppose I could send false, but I think that would be redundant.

Copy link
Contributor

@dotMorten dotMorten Dec 15, 2016

OK


if (!string.IsNullOrEmpty(value))
{
result = $"{result}&{parameterName}={value}";
Copy link
Contributor

@dotMorten dotMorten Dec 14, 2016

value should be Uri encoded

Copy link
Contributor Author

@SpilledMilkCOM SpilledMilkCOM Dec 15, 2016

These parameter should really be free-form text but I can add that just to be safe.

Copy link
Contributor

@dotMorten dotMorten left a comment

Looks good to me now

@deltakosh deltakosh merged commit bb22a72 into CommunityToolkit:dev Dec 15, 2016
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants