Skip to content
This repository has been archived by the owner on Feb 8, 2018. It is now read-only.

Fix delete elsewhere #2106

Merged
merged 3 commits into from Mar 1, 2014
Merged

Fix delete elsewhere #2106

merged 3 commits into from Mar 1, 2014

Conversation

zwn
Copy link
Contributor

@zwn zwn commented Mar 1, 2014

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

@Changaco
Copy link
Contributor

Changaco commented Mar 1, 2014

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.

@zwn
Copy link
Contributor Author

zwn commented Mar 1, 2014

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 one is going to raise the default iff the default is an exception. I had to look it up in the code. That is another one of the magic behaviors @whit537 seems to prefer so much and I find so surprising. That feature of one actually makes the code harder to grok 😢 IMO. It is not even documented in the 'raises' section of http://postgres-py.readthedocs.org/en/latest/#postgres.Postgres.one, you either have to go 2 pages down or it is actually faster to find in the code (it was for me).

@Changaco
Copy link
Contributor

Changaco commented Mar 1, 2014

@zwn

I don't think that optimizing for the size of the diff is worth of our time.

I wasn't trying to, it's just a nice side effect of re-cleaning the code.

And the cleanliness is in the eyes of the beholder.

Excessive line length is generally considered unclean.

That feature of one actually makes the code harder to grok 😢 IMO

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 ?

chadwhitacre added a commit to liberapay/postgres.py that referenced this pull request Mar 1, 2014
@chadwhitacre
Copy link
Contributor

It is not even documented in the 'raises' section of http://postgres-py.readthedocs.org/en/latest/#postgres.Postgres.one

Sorry. That's a doc bug and I've fixed it now.

That is another one of the magic behaviors @whit537 seems to prefer so much and I find so surprising.

Magic is also in the eye of the beholder. :-(

@chadwhitacre
Copy link
Contributor

Excessive line length is generally considered unclean.

We haven't published a style guide yet (#1485), but we agreed to 100-length lines. (Not sure what @zwn was using here.)

@zwn
Copy link
Contributor Author

zwn commented Mar 1, 2014

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.

@zwn
Copy link
Contributor Author

zwn commented Mar 1, 2014

Magic is also in the eye of the beholder. :-(

😉 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. 😄

@chadwhitacre
Copy link
Contributor

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. :-)

Changaco added a commit that referenced this pull request Mar 1, 2014
@Changaco Changaco merged commit 21d21ae into master Mar 1, 2014
@Changaco Changaco deleted the fix-delete-elsewhere branch March 1, 2014 13:46
@Changaco
Copy link
Contributor

Changaco commented Mar 1, 2014

@zwn Can you deploy, please ?

@zwn
Copy link
Contributor Author

zwn commented Mar 1, 2014

@Changaco Deployed. Verified working by deleting my Bountysource account.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants