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

Python 3 migration, #153 fix #174

Closed
wants to merge 21 commits into from
Closed

Conversation

kazeevn
Copy link

@kazeevn kazeevn commented Jul 18, 2014

Hello.

Facing with issue #153, I decided to try running hamster in python 3. After a myriad of small hacks, it works.

My current code is certainly not ready for production use - more like a proof of concept. Shall I continue working on the 2to3 migration?

waf isn't ported, so installation is partly manual:

./waf configure build --prefix=/usr && sudo ./waf install && sudo cp -r /usr/lib/python2.7/dist-packages/hamster/ /usr/lib/python3/dist-packages/

@tstriker
Copy link
Contributor

Hello!

Moving to py3 makes general sense. Wonder if there is anyway that the code code still run both on python2 and 3. Right now my local install has python 3 but not the whole gnome shebang (running fedora 20 + rhugh's preview packages, so it's gnome 3.12.2 hybrid), it seems, so i can't even run it.

two things i noticed in the pull request:

  • i did all the Gtk -> gtk imports intentionally, and am sticking with it - modules in python are all lowercase, gnome is on the odd side here, introspection or not.
  • the branch seems to be mixing whitespace adjustments, adding missing docstrings and adding instructions - all of those are welcome, but should be done in a separate branch. everything's that is not related to python3 migration should be done in separate branches.

@kazeevn
Copy link
Author

kazeevn commented Jul 19, 2014

Toms, thanks for encouragement.

Rewriting the code to run on both 2 and 3 is a possibility, but would be hard to achieve and maintain - think of the Unicode hacks.

Have you tried running the code? If so, how did it fail?

What's the deal with gnome? At a glance, the extension is pure JavaScript, so won't be affected.

About your code comments:

  • Agree about Gtk -> gtk . People expect the code to follow PEP8.
  • Changes mix. As I stated before, it would be very inconvenient for me to work with py2 version - I can't run trunk due to Encoding errors in non-ASCII locales #153 on Ubuntu 14.04. For now, I can just stick with making py3 code stable.

What are the prerequisites for the migration? I guess, usability testing, what else?

@tstriker
Copy link
Contributor

on my hybrid fedora 20 + gnome 3.12.2 it just couldn't find dbus for python3 - not much of a failure, just missing imports. i didn't look into that too deeply - that's the "what's the deal with gnome part". it could be a fedora thing (they intentionally skipped a cycle, not sure what ubuntu is up to, but i wouldn't like to fork code for distros as the life is miserable already as it is :) )

  • the utf fixes surely don't have anything to do with python 3 (i have to apologize that haven't looked into the bug as, frankly, hamster is not high in my priority list, but i'm running utf8 content myself)
  • while changes mix in your branch, you shouldn't mix them in the pull request - keep the pull requests relevant to the proposed change, while whitespace fixes might be ok, changing style like in src/hamster/widgets/timeinput.py serves no function - the code still does the same thing, or bugfixes like in widgets/facttree.py

the only prerequisite for migration would be that it runs on the most recent stable version of all modern distros.

@tstriker
Copy link
Contributor

herrm, so i think i would suggest you to sort out the utf8 issue in py2, and once you have trunk running, i'd be happy to apply the gazillion little fixes that you have in your fork, before we even look at they py3 migration - should make life simpler for everyone!

@kazeevn
Copy link
Author

kazeevn commented Jul 20, 2014

the utf fixes surely don't have anything to do with python 3

They do. One on of the major differences between py2 and py3 is streamlined Unicode support. Hacking soon-to-be obsolete py2 code looks like a waste of effort to me.

Obviously, I agree with that:

the only prerequisite for migration would be that it runs on the most recent stable version of all
modern distros.

But also I would like to point out, trunk doesn't work on Ubuntu 14.04 - recent and stable. Do you have any problem with python3-dbus on your installation?

while changes mix in your branch, you shouldn't mix them in the pull request - keep the pull requests relevant to the proposed change,

In principle this makes sense. However in this particular case, we don't make an incremental change, but a complex refactoring - it would be difficult to to separate bugfixes from actual 2to3 migration. Given that trunk commits are not very frequent, may I ask you to take holistic approach and review the changes altogether?

About the style in src/hamster/widgets/timeinput.py . It was done automatically by 2to3.py script and serves a purpose - in py3 filter(lambda, X) produces an iterator, in py2 a list, so the change preserved the code behaviour.

kazeevn@kazeevn-ub14:~/hamster$ ipython
Python 2.7.6 (default, Mar 22 2014, 22:59:56) 
Type "copyright", "credits" or "license" for more information.

In [1]: a=[1,2,3]

In [2]: filter(lambda x: x== 2, a)
Out[2]: [2]

kazeevn@kazeevn-ub14:~/hamster$ ipython3
Python 3.4.0 (default, Apr 11 2014, 13:05:11) 
Type "copyright", "credits" or "license" for more information.

In [1]: a=[1,2,3]

In [2]: filter(lambda x: x== 2, a)
Out[2]: <builtins.filter at 0x7fec5b46c320>

However, in many places an iterator would have sufficed, so I manually reviewed all the changes.

Is the issue with hamster-cli export present in py2 trunk?

@tstriker
Copy link
Contributor

not working on ubuntu 14.04 is news to me. there shouldn't be anything prohibiting doing that.

point taken on the wholistic approach, hope you also see my point on keeping the diff as minimal as possible.

export works fine here.

i'd still urge you to get your system running on python2 before going further, so you start with a working state and don't have to guesswork as to what's broken where. the utf fixes shouldn't take more than just maybe an hour.

now, i must apologize as i'll be gone for almost 4 weeks now (summer holidays et.al.) but it's not like hamster's going anywhere anyway :)

@kazeevn
Copy link
Author

kazeevn commented Aug 3, 2014

Have a nice holiday) Meanwhile I'll do my best (subject to time constraints) to produce a stable py3 version.

don't have to guesswork as to what's broken where

I guess, fixing a bug is good no matter who's to blame for its introduction ;)

@FelixSchwarz
Copy link
Contributor

What's the status here? @kazeevn would you mind if someone else drives this? I'm also in favor of fixing code incrementally (as @tbaugis suggested).

@axmachado
Copy link

Hello, people....

I believe I solved the #153 with a version that is working with python2.7... I'm willing to try to turn it into something that can run in python2 and python3, using @kazeevn code as a guide, in a more incremental aproach.

What do you think about it? Will it be a valid approach or is it better to try to merge the master branch with @kazeenv code and work backwards to make it python2 compatible? Any suggestions?

@FelixSchwarz
Copy link
Contributor

In general I like incremental changes which seems to favor making it Python 3 compatible while keeping Python 2 working. However I didn't check the code in this PR so maybe it's easier the other way around. So do as you feel fit :-)

@elbenfreund
Copy link
Contributor

elbenfreund commented Jul 6, 2016

Thank you for your feedback, it is much appreciated.

Project hamster and its various sub-components is currently undergoing some major changes. We prepare the introduction of a rewritten codebase for most of the underlying functionality. A direct consequence of this is that it is unlikely that any open/new bugs within the current/old codebase will be fixed (unless someone steps up and offers to do so) as most resources currently available will be invested in making the rewrite prime time ready.

If you are still interested in working on the new codebase (repositories: hamster-lib/cli/gtk/dbus) we would be most thrilled. Please feel free to either open a new issue with the relevant repository and/or join the discussion on the mailinglist.
For more context please see our website.

Thanks for your interest and support! Eric.

ederag added a commit to ederag/hamster that referenced this pull request Aug 22, 2018
Hopefully, fix utf-8 related issues projecthamster#153, projecthamster#239, projecthamster#317

Co-authored-by: Nikita Kazeev
Thanks for showing the way in PR projecthamster#174 !
@ederag
Copy link
Collaborator

ederag commented Nov 24, 2018

@kazeevn PR #363 has been merged,
applying the python3 migration commit 78c2867 that was closely guided by your proposition.
Since years passed, this was brand new 2to3 run,
together with minimal changes to get a working version.
Thank you very much for your important contribution !

@ederag ederag closed this Nov 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants