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

RFC: Repo cleanup, py2/py3 compatibility #131

Closed
wants to merge 8 commits into from

Conversation

bilderbuchi
Copy link
Contributor

This PR contains initial work towards a codebase simultaneously compatible with Python 2 and Python 3. Specifically, I used futurize to make the existing py2 codebase py3-compatible.
I have split this into separate commits so that thematically separable changes can be reviewed separately.

Note: If you want to review the changes without whitespace getting in the way, append ?w=1 to the relevant URL on Github to ignore whitespace changes, like this.

Note: The changes in here also contain the changes from PR #129, as it was easier to include those changes in the line ending normalization.

To start working with futurize, since I'm on Linux, I first had to normalize the line endings, otherwise all the files showed up as changed. This is because the repo contains CRLF (Windows-style) line endings, and no .gitattributes file to enable cross-platform line-ending compatibility. With the .gitattributes in place, this should now be transparent to all devs, without any special git settings, Windows users get CRLF endings on checkout, all the other get LF. (I already handled such a transition for a bigger project)
This work is contained in commit 2216fa2.

The next set of change is what future calls "stage 1", easy replacements with very low bug potential and no additional dependencies. cb1fc53 dfab1e6

After those are the autogenerated stage 2 changes, which fix more incompatibilities, but add a dependency on the future package. A small amount of manual fixes, which the futurize heuristics didn't pick up, were also added. 342fe51 d5b9cbe

FYI: I also have a branch with an initial set of pep8 fixes, but I have not included this here yet since I don't know how you folks feel about pep8. Also, as there is already a commit in here that touches _all the files, we might as well fix the style issues at the same time._

I know that PR #120 is currently open, and not included in these changes. This is not ideal w.r.t the line ending normalization, but most of the changes in here happened with terminal commands, so it's easily replayable once that PR is merged (if it gets merged).

All the tests pass, and all the examples that worked before, keep working with those changes. Tested only on py2, since there's no wxpython for py3 yet.

On a TODO-list for this PR:

  • Fold in the pep8 changes if acceptable
  • (probably) set up a Travis-CI account to get started with at least some automated testing (pytest, plus maybe run some examples?).

Ping @chriskiehl
Relevant issue: #65

config_generator.has_argparse could not work as
`f` does not exist in scope. Furthermore, I suspect it is
probably a duplication accident, as it's very similar
to source_parser.has_argparse, which is the only one
used in the codebase.
These are safe fixes which shouldn't break py2 compatibility.
These were autogenerated with "futurize -2wn .", manual fixes follow.
All tests pass, examples work as before. New dependency: future
This was referenced Dec 2, 2015
@bilderbuchi
Copy link
Contributor Author

@chriskiehl any feedback on this?

@chriskiehl
Copy link
Owner

Hey @bilderbuchi!

I haven't had a chance to review this in detail yet (it's a big diff!), but don't think that I'm ignoring it. Just a few things ahead of it in my queue of stuff to do!

@bilderbuchi
Copy link
Contributor Author

sure, no problem. Keep in mind that I'll have to re-do the changes anyway, as other PRs got merged in the meantime, but it would be great to get any discussion out of the way first, before re-doing, then merging.
Also, it makes sense to review commit-by-commit (it's only 5 commits, effectively), they're thematically isolated (as detailed in the PR description).

@bilderbuchi
Copy link
Contributor Author

@chriskiehl have you had a chance to review this already? how can I help move this PR (and Py3 compatibility in general) forward?

@chriskiehl
Copy link
Owner

Hey @bilderbuchi,

Unfortunately, odds of this PR moving forward anytime soon are pretty slim. Gooey is currently a WX project, and that is a fundamental limitation on which Python versions can be targeted. Given that fact, I don't see a compelling reason to introduce the maintenance overhead of 2to3 conversion -- at the end of the day, we're still stuck on a 2.x code base.

I made some strides to prep Gooey for a new tool kit -- stuff like (finally!) ripping logic out of the views and doing some general project clean up. But alas, contract work is eating up 99% of my free time. Working on Gooey means falling behind on paid work, so I've been trying to keep it to a minimum ;) Until those end, bringing real Python 3 compatibility via a new GUI kit isn't happening.

So, in short, it'll probably be a few months before anything other than bug fixes happen on this project : (

Sorry, man!

@bilderbuchi
Copy link
Contributor Author

hey @chriskiehl
I'm not sure if that is a big maintenance overhead, the majority of the changes is using the print function instead of statements, and some imports from builtins to make code work for both 2 and 3. Also, since from your comment I'm not sure if you picked this up, but this PR creates code that runs on both 2 and 3 withouth modification, so 2to3 is not involved, and therefore does not create overhead (in setup.py or wherever).

Also, at the end of the day, the work done here is prep work for future 3 compatibility, just like your prep work for a new tool kit, and will have to be done anyway whenever you want to move forward with 3 compatibility.

Btw, I have heard recently that wx phoenix snapshots are already usable, so maybe WX on Py3 is not that far off, actually.

In the end, I guess it boils down to one question: Do you want outside help in moving Gooey towards Py3? I can see myself working towards that goal (and I already did with this PR), but if you can't review (and hopefully merge) PRs I throw your way, I'd better not bother.

Btw, do you know of other packages that do something like what Gooey does? I have recently introduced Python at work (research, not programming), and I think my colleagues could find Gooey useful for on-and-off scripting work, but I won't fall back to Py2 just for this one package.

finally, the line ending changes are totally decoupled from py3 compat, and should be done anyway to have a "proper" structure. I guess I should have sent a separate PR for them.

@bilderbuchi
Copy link
Contributor Author

Also, because this is textual communication after all, I wanted to clarify that I don't want to pressure or blackmail into doing something not sustainable on your side, I just want to calibrate my expectations, and want to avoid that we waste each other's time. :-)

@lathomas64
Copy link

was excited to hear about this but am using python 3 so sadly can't use it in the current state, would be glad to see this version if it works with 3 become adopted

@bilderbuchi
Copy link
Contributor Author

@lathomas64 it's better to contribute over at the originating issue #65, if this PR gets closed any discussion will not be very visible anymore.

@chriskiehl
Copy link
Owner

Closing. A partial rewrite occurred which invalidated most of these changes.

I now use 4 spaces rather than two, though! So, half-way to PEP8 ^_^

@chriskiehl chriskiehl closed this Jan 28, 2018
@bilderbuchi
Copy link
Contributor Author

Fair enough! :-)

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.

None yet

3 participants