search

pierrepo / PBxplore

A suite of tools to explore protein structures with Protein Blocks :snake:
https://pbxplore.readthedocs.org/en/latest/
MIT License
28 stars 17 forks source link

Fix few bugs and bump to v1.4.0 #175

Closed gabrielctn closed 3 years ago

gabrielctn commented 3 years ago
pierrepo commented 3 years ago

Hi @gabrielctn :wave:

Could you explain if this PR preserve or breaks back compatibility with Python 2?

All tests in CI failed because of

The command "sh -e /etc/init.d/xvfb start" failed and exited with 127 during .

It may worth we move to GitHub actions. @HubLot could you please help us with this?

HubLot commented 3 years ago

Hi,

Thanks for this PR ! It's great to have some improvements.

I agree with @pierrepo we should move to actions! I gonna look into it. I prefer to not merge this until this is done.

I'm not a big fan of having merge commits from a fork into the original repo but it's not a big deal either. Maybe, for the next time, you can propose PR directly here. All of us will be happy to merge :)

If it breaks python 2 compatibility (which I don't mind), the new version should be 2.0.0 or at least 1.4.0.

pierrepo commented 3 years ago

Thanks @HubLot for your input.

I don't mind either if we break the Python 2 compatibility but we indeed must state it clearly (version 1.4.0 sounds good). If we move forward, I'll add in the Citation section of the README.md that version 1.3.8 corresponds to the paper and will link to Zenodo and Sofware Heritage warehouses.

HubLot commented 3 years ago

I updated the CI to GitHub Actions in PR #176. Besides the issue with Weblogo import name, it's working but with the lost of Python 2.7 and 3.4

So, I suggest to first merge this PR (once questions raised solved) and after mine.

gabrielctn commented 3 years ago

Some of the changes for python 3 seem unnecessary and automatically applied. Not all range need to be wrapped in list, and not all print need more parentheses.

Hi everybody !

Sorry nevermind, I used 2to3 script which automatically applied these patches ("list" and double parentheses for print functions) apparently I should have added some options to the script to avoid unwanted behavior (https://stackoverflow.com/a/55559891/6401758) I commit the changes to revert these changes right away.

@pierrepo It indeed drops compatibility with Python 2 (as it should I guess :wink:) Ok to bump to version 1.4.0

I'm not a big fan of having merge commits from a fork into the original repo but it's not a big deal either. Maybe, for the next time, you can propose PR directly here. All of us will be happy to merge :)

Well, I started working on this in Alexandre's fork of the repo, so I thought I would finish there. Yet, eventhough I kind of agree with you because it is mush simpler to work on the original repo, I also agree with @jbarnoud on the fact that it is an intended use of git. Anyway, as long as it works.

HubLot commented 3 years ago

Thanks @gabrielctn for the new commits.

From what I see, here is what remains to be done:

jbarnoud commented 3 years ago

It is not only the README that needs to be made aware of the python that are supported. If we want to play it modern, then we need to tell setup.py as well: https://packaging.python.org/guides/distributing-packages-using-setuptools/#python-requires

gabrielctn commented 3 years ago

Thank you for your reviewing ! All tests are passing on my side, if you see something missing please tell me, otherwise we're good to merge.

HubLot commented 3 years ago

Good to me too!

pierrepo commented 3 years ago

Looks very good to me too. Maybe I should merge #176 first.

HubLot commented 3 years ago

Readthedocs still has the previous version. I don't know if we need to trigger it or if it's done automatically. @pierrepo could you look into it ? Don't forger also to update the CITATION section :)

pierrepo commented 3 years ago

Readthedocs still has the previous version. I don't know if we need to trigger it or if it's done automatically. @pierrepo could you look into it ?

For some unknown raisons, I had to create the build of the doc manualy :astonished: The doc is now up-to-date!

Don't forger also to update the CITATION section :)

On my way.