sseemayer
Scope issues
I tried to fix issue #2 myself, but found many places where you have defined a function within a method. Are you doing something tricky with scope that I shouldn't mess up, or can I pull these functions out to the module level?
It doesn't seem to make sense to define multiple functions every time a module is called. Nor to have variables in a function that are only defined in the parent namespace. Better to pull the function out. And if you want to make a specialized version of the function, you can use functools.partial.
This issue is difficult to solve myself because you have the same subfunction name defined in different methods, with different signatures. So the task of pulling out all the functions is challenging without knowing if there is a method to this madness.
Hi hsharrison,
the functions are mostly defined there so that it becomes clearer where they are used. I didn't anticipate anyone using them outside of the respective methods, but if you believe that they are of use, you are welcome to improve the code. There could be a little bit of scope stuff going on but that should be easily resolvable.
To be honest, I was still getting rid of my habit of putting everything into a class Java-style programming when doing Py2D. A lot of the code could probably be made more pythonic :)
Best, Stefan
Well, I tried to do so. You might want to check out my branch at https://github.com/hsharrison/Py2D
I'm not sure how easy it would be to encapsulate my changes into a pull request, since my IDE changed tabs to spaces in all the files I worked on (so diffs will be hard to read). I didn't get a chance to test all the methods I pulled functions out of either.
I also worked on the other issue I posted, changing staticmethods to classmethods wherever possible, and added a few more "magic" methods, e.g. polygon + vector and vector * scalar.
I think all your improvements so far are great! The only thing I'm unsure about is the change in indentation style. Changing it either way would be messy since we both have histories in different styles - would you be willing to switch to tabs?
Hi all, Sorry to intervene so brutally, but please no tabs.
I don't have much opinion on either tab or space, it is just that one of them is somewhat winning (space over tab) and the community sort of agrees on this topic (cf Europython). Let's move to some more interesting stuff. My 0.02c
Sorry, again, for the devil out of the box. L.
On Mon, Nov 18, 2013 at 4:57 PM, Stefan Seemayer [email protected]:
I think all your improvements so far are great! The only thing I'm unsure about is the change in indentation style. Changing it either way would be messy since we both have histories in different styles - would you be willing to switch to tabs?
— Reply to this email directly or view it on GitHubhttps://github.com/sseemayer/Py2D/issues/3#issuecomment-28709855 .
Cordialement, Lionel Barret,
LBdN Consulting
http://www.lbdn-consulting.com
LinkedIn Profile : http://www.linkedin.com/in/lionelbarretdenazaris
Viadeo : http://fr.viadeo.com/fr/profile/lionel.barretdenazaris
Membre de l'Arsenal Numérique http://arsenal-numerique.org/
In the meantime, I also prefer spaces. I've only shied away from changing indentation style for the sake of backward compatibility, but if everyone wants spaces we can of course switch. It doesn't look like there are any other active forks that we would break...
It shouldn't affect backward compatibility as long as it's consistent within each file (I think).
That said, I'm not too confident that my changes can be dropped in whole-hog. Some of my changes are potentially not backwards compatible, such as changing some methods to properties. Some of these you did yourself by assigning to the property function, but if anyone's code is calling get_* or set_* directly it won't work anymore. Also I think I changed one or two that you did not previously have as properties so those will be broken going forward (actually, on second thought, maybe it works if you call a property with no inputs? Never tried it...)
Finally I'm fairly new to Python and I've never written code for anything other than 3.3, so I can't promise that my changes are compatible with 2.x.
For these reasons you might want to take a close look at what I wrote before you incorporate it into the main branch. If there are changes I can make in my branch to help the process, let me know (I'm new to this whole open source contributing thing as well, not exactly sure how everything works).
Whoops, sorry, I thought I clicked the 'reset form' button on the comment there.
I think the minor API changes so far shouldn't be too much of a problem, I was mainly wondering about losing progress of potential other contributors (but there don't seem to be any public forks so I declare that there aren't any). Edit: We could maybe add the old getter functions back with a deprecation warning and then drop them in a month or so, but I think that people should be able to figure it out.
Regarding your changes, everything looks nice so far and I'll be happy to try stuff out in Python 2 and 3. It would help me if you could split the commit at hsharrison/Py2D@19be7a4e1e58b48caee2409ffd54508ff78f8f7c into two separate commits (indentation and other) so I can actually see what's going on.
Err, I've been reading a little on how to do this and I'm seeing notes like this:
Warning: It is considered bad practice to rebase commits which you have already pushed to a remote repository. Doing so may invoke the wrath of the git gods.
Is it okay since no one, presumably, has pulled from my repository?
Correct, see e.g. http://stackoverflow.com/questions/19096170/pushing-to-remote-after-locally-rebasing-commits
Just to update you... I finally came back to this project and found that I had introduced some errors in my fork, so I started over. All I have now is a couple of fixes I needed to get intersections working in Py2D. I hope I didn't delete anything useful in my fork but I believe all the changes I made were trivial. Also, now that I'm using composition instead of subclassing to work with Polygons I don't require as many changes. There are still a few issues which I will post shortly.