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

Refresh #12

Merged
merged 7 commits into from
Nov 3, 2016
Merged

Refresh #12

merged 7 commits into from
Nov 3, 2016

Conversation

brad
Copy link
Member

@brad brad commented Nov 3, 2016

@orcasgit/orcas-developers Please review. I updated the tox matrix and added docstrings to all the models and help_texts to all the model fields.

@coveralls
Copy link

coveralls commented Nov 3, 2016

Coverage Status

Coverage remained the same at 98.081% when pulling 8576ba7 on refresh into d41e590 on master.

Copy link
Contributor

@grokcode grokcode left a comment

Choose a reason for hiding this comment

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

Looks great! I have just a couple suggestions on the help_texts.

calories = models.FloatField(help_text='Calories for the day')
activity_calories = models.FloatField(
help_text='Activity calories for the day')
distance = models.FloatField(help_text='Distance travelled during the day')
Copy link
Contributor

Choose a reason for hiding this comment

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

@brad It might be helpful to put the units here.

Copy link
Member Author

Choose a reason for hiding this comment

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

@grokcode Good call, the documentation doesn't specify but it's probably miles. That would fit well with the examples and it looks like distance on other API end points come in miles.

user = models.ForeignKey(UserModel, help_text="The device's user")
device_type = models.CharField(
max_length=64,
help_text='The device type. At this time, will always be "shine"')
Copy link
Contributor

Choose a reason for hiding this comment

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

@brad I know Misfit's docs say this, but there are some other values that Misfit will send besides shine. #9

Copy link
Member Author

Choose a reason for hiding this comment

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

@grokcode Oh, I forgot, thanks!

@coveralls
Copy link

coveralls commented Nov 3, 2016

Coverage Status

Coverage increased (+0.03%) to 98.109% when pulling bf2ae48 on refresh into d41e590 on master.

@grokcode
Copy link
Contributor

grokcode commented Nov 3, 2016

LGTM! Thanks for this update! :shipit:

@grokcode grokcode merged commit 9704512 into master Nov 3, 2016
@grokcode grokcode deleted the refresh branch November 3, 2016 17:15
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

3 participants