-
Notifications
You must be signed in to change notification settings - Fork 308
Conversation
I have added a commit to re-clean the code, the PR's diff is much smaller now. @zwn If you have no objection to the change I've made, I'll merge the PR. |
I don't think that optimizing for the size of the diff is worth of our time. And the cleanliness is in the eyes of the beholder. That said, I had no idea that |
I wasn't trying to, it's just a nice side effect of re-cleaning the code.
Excessive line length is generally considered unclean.
Maybe, but we already use it elsewhere in the code, so if you want to remove it I think you should open a separate issue/PR. So, should I merge this PR or do you find my changes unacceptable ? |
Pointed out by @zwn at gratipay/gratipay.com#2106 (comment)
Sorry. That's a doc bug and I've fixed it now.
Magic is also in the eye of the beholder. :-( |
I just don't want to spend more time on this than necessary. Please go ahead with anything you see fit. Just make sure it does what it did before the changes. |
😉 Well, POSTing to elsewhere.json to delete something is surprising and raising param called default is not? Think of the poor souls that are not the authors of postgres.py. 😄 |
I try but I fail. That's why we need actual souls using it and providing feedback. :-) |
@zwn Can you deploy, please ? |
@Changaco Deployed. Verified working by deleting my Bountysource account. |
During the merge with #1369 we have lost the ability to delete non sign-in elsewhere accounts. This PR adds a test for this functionality and a fix as well.
ref #639