Skip to content
This repository has been archived by the owner on May 13, 2022. It is now read-only.

Blame preserving reformat - simple merge #340

Closed
wants to merge 0 commits into from

Conversation

ghtdak
Copy link
Contributor

@ghtdak ghtdak commented Dec 1, 2015

This PEP-8 / Google Yapf reformat preserves the blame history of the project and is a straight merge with the Joinmarket master (In contrast with the tree replacement strategy discussed previously)

This pull-request contains no code changes other than Yapf reformats though is an important starting point to begin the process of refactoring joinmarket.

ghtdak/master contains the squashed rebased merge of the parallel (universe?) reformatted project history exploiting some git-fu:

git merge --squash -X theirs master-yapf

This reformat fixes many of the problems of the unformatted and very "tool unfriendly" aspects of the codebase (most notably tabs... ugh...:-)

@chris-belcher
Copy link
Collaborator

I'd like to do this. I think each line will have to be looked over before merging though. I could find the time to do this this week I think.

@ghtdak
Copy link
Contributor Author

ghtdak commented Dec 2, 2015

Its easy to re-create the final state of ghtdak/joinmarket/master from belcher/master or JoinMarket-Org/master using yapf

pip install yapf
cd <repo>
yapf -i -r --style google .

I recommend tig for viewing the git repo though PyCharm Community Edition is better for detailed git diff analysis of python.

I've run yapf on some pretty big codes (e.g twisted) and it doesn't change the results of trial or tox. YMMV but I'd be surprised if it broke Joinmarket.

Note: I used google style which is slightly different from the yapf PEP-8 baseline. I'm sure I have a good reason :-)

meh...

@AdamISZ
Copy link
Member

AdamISZ commented Dec 2, 2015

First check through: used git diff -w. This throws up only cases where the lines have been broken up. Of course, reviewing that manually was .. interesting :) .. so obviously I didn't attempt to do it perfectly, but used a "heuristic" approach. At least, it threw up the amusing fact that there are 2 lines ending in semicolons that get removed. Another thing thrown up: old_mnemonic.py word list is one word per line, this is corrected to several words per line. Is there any obscure reason I can't think of why it's helpful to stick to one word per line? Almost certainly it doesn't matter.

This version of the code passes regtest.py and wallet-test.py.

The last check I did was this: create a fresh copy of the JoinMarket-Org/master, apply the yapf command above (note: --style not -style), and do a diff on the entire directory tree, to this PR. There were no differences, except the following two files:
lib/bitcoin/bci.py
lib/bitcoin/ripemd.py

Not sure the logic of not applying the changes to these files?
Apart from that, the check passed: diff -r showed only difference in git files, not in any code.

Short version; ACK.

Last comment, I want to say how much I appreciate this work @ghtdak , not just for the obvious reason, but also because looking at the updates helps me to understand more concretely what's needed (well, at least at the formatting level).

@AdamISZ
Copy link
Member

AdamISZ commented Dec 2, 2015

Also passes tumbler-test script. This is a significantly richer test, using 8 participants all with wallets randomly filled across 5 mixdepths, and 16 transactions. Of course, these tests do not cover that much even so.

@ghtdak
Copy link
Contributor Author

ghtdak commented Dec 2, 2015

The last check I did was this: create a fresh copy of the JoinMarket-Org/master, apply the yapf command above (note: --style not -style), and do a diff on the entire directory tree, to this PR. There were no differences, except the following two files:
lib/bitcoin/bci.py
lib/bitcoin/ripemd.py

Yeah, something happened with those two files during the squash squash merge. The resulting code seemed fine so I didn't sweat it though its on my queue of things to look at (priority just got higher :-)

WRT the line splitting... I decided to use the google style because it seemed to do a nice job and I figured that would be their emphasis. However, there are many knobs which can be tweaked, many of them continuous variables. Yapf a scoring system to make certain decisions and that can be played with.

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.

3 participants