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

added support for optional [date] parameter for MFP plugin #174

Open
wants to merge 3 commits into
base: gonzobot
Choose a base branch
from

Conversation

edillingham
Copy link

uses python-dateutil to parse date strings instead of requiring a strict format

@edwardslabs
Copy link
Owner

For this one instead of adding a new requirement could you see if you could use the timeformat.py and timeparse.py in cloudbot/util/?

It does a nice job of formating time strings. Check remind.py for examples.

@edwardslabs edwardslabs closed this Oct 4, 2017
@edwardslabs edwardslabs reopened this Oct 4, 2017
@edwardslabs
Copy link
Owner

Whoops didn't mean to close this.

@edillingham
Copy link
Author

edillingham commented Oct 4, 2017

It doesn't look like cloudbot.util.time_parse supports anything to do with dates, which is what the plugin requires. I can remove the requirement and add a custom date parser if that's what the project prefers, but should we really reinvent the wheel?

request = requests.get(scrape_url.format(text))
"""
<user> [date]- returns macros from the MyFitnessPal food diary of <user>
optionally, specify [date] to retrieve that day's diary

Choose a reason for hiding this comment

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

The core will only grab the first line of the docstring for command documentation, you'll want to combine these lines.

"""
date = 'today'

args = text.split()

Choose a reason for hiding this comment

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

I'd recommend doing text.split(None, 1) to limit the split to only splitting once, since you only want to retrieve 2 parts from text.

args = text.split()
user = args[0]
if(len(args) > 1):
dt = parser.parse(' '.join(args[1:]))

Choose a reason for hiding this comment

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

To go along with my comment about the .split(), you wouldn't need to use a join if the text is only split on the first space.


args = text.split()
user = args[0]
if(len(args) > 1):

Choose a reason for hiding this comment

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

Redundant parentheses in if statement. Format should be if len(args) > 1:


from cloudbot import hook

scrape_url = "http://www.myfitnesspal.com/food/diary/{}"
scrape_url = "http://www.myfitnesspal.com/food/diary/{}?date={}"

Choose a reason for hiding this comment

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

It would probably be a good idea to use the parameters keyword to requests.get() to pass query parameters rather than formatting them in to the URL. For example:

request = requests.get(scrape_url.format(user), parameters={'date': date})

which ensures that the parameter is properly escaped.

@@ -45,7 +57,7 @@ def mfp(text, bot):
output += ("{caption}: {total}/{remain}{units} ({pct}%) "
.format(**kwargs))

output += " ({})".format(scrape_url.format(text))
output += " ({})".format(scrape_url.format(user, date))

Choose a reason for hiding this comment

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

This should probably use request.url instead of duplicating the URL generation code.

edwardslabs added a commit that referenced this pull request Jan 31, 2018
Sort cmdinfo and potential match help outputs
@linuxdaemon
Copy link

All PRs for gonzobot should be submitted to snoonetIRC/CloudBot now.

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.

3 participants