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

Remove python future compatibility library and remove more python 2 compatibility code. #525

Closed

Conversation

jameshilliard
Copy link
Contributor

No description provided.

@kristapsk
Copy link
Member

Some tests are failing for me with this, unless #538 is applied on top, so I think that should be merged before more reviewing testing is done here (platform linux -- Python 3.6.5, pytest-5.4.1, py-1.8.1, pluggy-0.13.1 with failing tests).

@AdamISZ
Copy link
Member

AdamISZ commented Mar 14, 2020

Just double checked - indeed there are test failures with Python 3.6.8, pytest-5.4.1, py-1.8.1, pluggy-0.13.1 - but that's exactly as expected, no PRs will pass with 5.4.1 as I was mentioning I think on IRC. #538 is going to be needed for any test to pass from now on - and even that won't be enough because it appears our bitcoind download process on travis is also failing (entirely separate phenomenon which has cropped up at the same time). I checked also that with pytest 5.3.5 this PR is passing.

@AdamISZ
Copy link
Member

AdamISZ commented Mar 14, 2020

So after looking at this a bit more, I think it's not necessary to do this before #536 , and is more work that way.

Reasoning: apart from about 3 template changes (struct.pack no "b", iteritems->items, removal of "future"), everything substantial is changes in jmbitcoin, and the vast majority of that code is deleted, and even in a few other cases I've already simplified/removed conversions that you're editing here. The template-like changes in jmclient can easily be added later.

Sorry, realise this kind of thing is rather annoying but given how much work #536 has been, I don't think it makes sense to interrupt it with a potentially confusing rebase for a fairly minor change, that will need a whole new round of testing and analysis in case something is off.

@jameshilliard
Copy link
Contributor Author

the vast majority of that code is deleted

I assumed that would make it relatively easy to rebase #536 on top of this. Once #536 is merged I can rebase this PR fairly easily anyways, I just figured removing the last of the python future weirdness would simplify testing of #536.

@AdamISZ
Copy link
Member

AdamISZ commented Mar 14, 2020

I just figured removing the last of the python future weirdness would simplify testing of #536.

Yes that's a reasonable thought, and thank you, but it's now kinda all done anyway.

AdamISZ added a commit that referenced this pull request Apr 30, 2020
This is in line with #525 and corrects erroneous
addition of more compatibility code.
AdamISZ added a commit that referenced this pull request May 1, 2020
This is in line with #525 and corrects erroneous
addition of more compatibility code.
AdamISZ added a commit that referenced this pull request May 4, 2020
This is in line with #525 and corrects erroneous
addition of more compatibility code.
@AdamISZ
Copy link
Member

AdamISZ commented May 8, 2020

Closing this on the basis that it's made redundant by #536 (although many thanks for correcting errors there as you have done).

@AdamISZ AdamISZ closed this May 8, 2020
AdamISZ added a commit that referenced this pull request May 11, 2020
This is in line with #525 and corrects erroneous
addition of more compatibility code.
AdamISZ added a commit that referenced this pull request Jun 6, 2020
This is in line with #525 and corrects erroneous
addition of more compatibility code.
AdamISZ added a commit that referenced this pull request Jul 4, 2020
This is in line with #525 and corrects erroneous
addition of more compatibility code.
AdamISZ added a commit that referenced this pull request Jul 6, 2020
Update no-history-sync code:
This updates the new functionality in jmclient.wallet_utils
in the no-history-sync PR #444 to be compatible
with the python-bitcointx refactoring.

Remove all future/py2 compatibility code remaining:
This is in line with #525 and corrects erroneous
addition of more compatibility code.

Addresses all flake8 complaints (ununsed imports etc)

Addresses review of @dgpv

Addresses review of @kristapsk
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