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

Monzo Pots #13

Merged
merged 13 commits into from
Jan 27, 2018
Merged

Monzo Pots #13

merged 13 commits into from
Jan 27, 2018

Conversation

Sheaffy
Copy link
Contributor

@Sheaffy Sheaffy commented Jan 19, 2018

I have added the ability to view the currently authorised user, the ability to see the pots that they have, along with all their data.

@coveralls
Copy link

coveralls commented Jan 19, 2018

Coverage Status

Coverage decreased (-4.05%) to 91.262% when pulling bfec028 on Sheaffy:master into 2ed4b23 on pawelad:master.

@Sheaffy
Copy link
Contributor Author

Sheaffy commented Jan 19, 2018

Just realised that someone else has also pulled in monzo pots.
I did however add pots to the API objects :)

@pawelad
Copy link
Owner

pawelad commented Jan 19, 2018

Awesome, thanks for the PR!

The CI failure is because of flake8 (you can see the actual errors in Travis CI log) - could you please fix that? I'll also need the test for this endpoint, along with the vcrpy cassete before I merge this - you can either do that yourself (I'll be happy to help) or leave it to me (which will probably take a little bit more time), entirely up to you.

@Sheaffy
Copy link
Contributor Author

Sheaffy commented Jan 19, 2018

Thats no problem I can do all of that..

@pawelad pawelad self-requested a review January 19, 2018 17:02
@pawelad pawelad self-assigned this Jan 19, 2018
@pawelad pawelad added the enhancement New feature or request label Jan 19, 2018
@pawelad pawelad added this to the v0.11.0 milestone Jan 19, 2018
@pawelad
Copy link
Owner

pawelad commented Jan 19, 2018

Awesome, just remember to redact your data from the cassete : )

@Sheaffy
Copy link
Contributor Author

Sheaffy commented Jan 19, 2018

Okay, tox is now passing on flake8, will do the endpoint test now.

@coveralls
Copy link

coveralls commented Jan 19, 2018

Coverage Status

Coverage decreased (-4.008%) to 91.304% when pulling 16c6948 on Sheaffy:master into 2ed4b23 on pawelad:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 95.169% when pulling 25379c1 on Sheaffy:master into 2ed4b23 on pawelad:master.

@coveralls
Copy link

coveralls commented Jan 19, 2018

Coverage Status

Coverage decreased (-0.1%) to 95.169% when pulling f03a580 on Sheaffy:master into 2ed4b23 on pawelad:master.

@Sheaffy
Copy link
Contributor Author

Sheaffy commented Jan 20, 2018

@pawelad We should be all good to go now, a -0.1% decrease on coveralls though :(

def pots(self, refresh=False):
"""
Returns a list of pots owned by the currently authorised user.
Official docs:
Copy link
Owner

Choose a reason for hiding this comment

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

100% a nitpick, but would you mind adding empty lines before Official docs: and :param refresh: so it's in the same format as other docstrings? Thanks!

Copy link
Owner

@pawelad pawelad left a comment

Choose a reason for hiding this comment

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

All this looks awesome, thank you!

@pawelad
Copy link
Owner

pawelad commented Jan 26, 2018

And sorry it took me almost a week - I was planning to deal with the coverage decrease as well but didn't find the time to do it and it shouldn't block this anyway. I'll try to open a PR for the couple conditional branches (if not account_id and if not refresh) on the weekend and do a proper release, possibly with monz update as well.

@pawelad pawelad merged commit a315dd0 into pawelad:master Jan 27, 2018
@pawelad pawelad mentioned this pull request Feb 15, 2018
@pawelad
Copy link
Owner

pawelad commented Feb 16, 2018

Released in v0.11.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants