search

aarondcoleman / Fitbit.NET

Fitbit .NET API Client Library
MIT License
197 stars 140 forks source link

Added new API methods to FitbitClient #198

Closed redmorello closed 7 years ago

redmorello commented 7 years ago

I've aded the following API methods to the FitbitClient:

LogActivityAsync = To enable us to post new activities to Fitbit.

GetActivityLogsList = To retrieve a list of activity logs.

GetHeartRate = Multiple methods to get Heart Rate Time Series and IntraDay Time Series data.

I've not added any unit tests, as this is something I've not done before.

kchanakira commented 7 years ago

They're not accepting PRs without at least 1 unit test, and 1 integration test.

kehollin commented 7 years ago

I might have some time to do it as I need the heart rate stuff, talking to Aaron now about that.

kehollin commented 7 years ago

Also, my knowledge of unit tests is lacking too. My plan was to find a pre-existing function, search the test projects for that method, then do what I do best, copy and adapt. :)

aarondcoleman commented 7 years ago

@kehollin we'd love to pull this in but it needs unit tests around its ability to successfully parse a sample JSON response for the data retrieval methods. I don't really have an easy way to test if it successfully posted, data to Fitbit, so less concerned there. If you can take a stab at adding these then we can get them merged in quickly.

kehollin commented 7 years ago

@aarondcoleman Got it, not sure when I'll be able to get it finished, will let you know after I've done an assessment of the changes. IIRC it's fairly simple, similar to the work I did for our shared Jawbone library back in the day (like 2 years ago). @redmorello I've gone into your repo and essentially copied your pull request to Aaron to my fork and it merged OK. Not sure if GitHub would show you that activity or not, but wanted you to know.

redmorello commented 7 years ago

I did make a start on adding some unit tests, I'll try to commit those files when I'm in the office tomorrow.

redmorello commented 7 years ago

@aarondcoleman I've not done unit tests before, so please let me know if they need amending :)

redmorello commented 7 years ago

@WestDiscGolf those changes have been made apart from the negative value check. Can you explain more what your trying to check for??

redmorello commented 7 years ago

@WestDiscGolf have put in a check for limit.

aarondcoleman commented 7 years ago

Hi @redmorello -- we tried to take this PR but found there to be a lot issues with the model code hierarchy you used for ActivityList. We also found a huge hole / bug with the way this specific API handles timezones and ended up doing a ton of investigation on our end which led us to a custom parsing technique and returning DateTimeOffset instead. Because of that this PR now isn't consistent with the direction of the codebase at master.

See @dwaynefitabase update here: https://github.com/aarondcoleman/Fitbit.NET/pull/233

If you'd like to resubmit it again to add in the ability to log activity that'd be great. I know there were also some additional heart rate related endpoints. These should all be separate PRs, as small a unit of change as possible so that we can evaluate each individually and more clearly. So, log activity, hr, anything else cleanup, should all be independent PRs.

Cheers, --Aaron