search

SolidCode / SolidPython

A python frontend for solid modelling that compiles to OpenSCAD
1.1k stars 172 forks source link

Minimum Python Version supported? #116

Closed Cabalist closed 4 years ago

Cabalist commented 4 years ago

I see that the CI runs only on Python 3.7. I'd like to go through and add some typing to the codebase but I don't know what the minimum version of Python is supported.

I'm seeing f-strings in the code. Would it be a welcome PR to add in type hints?

etjones commented 4 years ago

For a long time we were supporting 2.7, but I've made some changes (mostly f-strings, like you mentioned) that will mean we're only 3.7+. I'm fine with that.

Normally I'd totally welcome a type hinting PR! But... I've already done most of that work. See the feature/type_hints branch. I may still need to do a little cleanup there, but I think it's mostly ready to merge to master. Thanks for the offer, and for the prompt to get things moving!

Cabalist commented 4 years ago

That's great! I was looking at master and should have taken a closer look around. I see a number of other bugs are fixed in that branch as well. :)

What is the best branch for me to look at if I'm looking for the latest?

etjones commented 4 years ago

Well, master is where I've been putting most recent changes. I just started merging with the feature/type_hints branch. If that goes smoothly, I'll push that to master shortly.

If you're looking at that code right now, I'd be grateful for another pair of eyes on how I did the annotations. Specifically, I added a lot of aliases so that method signatures didn't get too crazy. (e.g. def __add__(self, x): => def __add__(self, x:Union[OpenSCADObject, Sequence[OpenSCADObject]]) -> OpenSCADObject:)

So, that shortens & clarifies things some, (def __add__(self, x:OScOPlus) -> OScO:) but I was a little concerned about all the gymnastics I did to get those types working & not overlong. If you get a chance, I'd welcome any comments about how I did the annotations in objects.py and solidpython.py; this is the first big bunch of annotation I've done, and I want it to be clear to anyone reading the code. Cheers!

Cabalist commented 4 years ago

Yeah! Most of this looks legit.

First impression is that I would probably leave OpenSCADObject as itself instead of OScO for clarity. I see it also defined as OSC_OB = OpenSCADObject in solidpython.py.

If it is a matter of circular imports you can use the TYPE_CHECKING flag in typing to only import things if it is not being executed. Also you can use string literals where need (i.e. ... -> "OpenSCADObject")

etjones commented 4 years ago

Thanks for taking a look! Yeah. I felt a little dirty with the naming and renaming bits; best not to let my sloppiness live on. I'll make a pass through and get everything cleaned up, then merge to Master. Cheers!

On Oct 10, 2019, at 4:57 PM, Ryan Jarvis notifications@github.com wrote:

Yeah! Most of this looks legit.

First impression is that I would probably leave OpenSCADObject as itself instead of OScO for clarity. I see it also defined as OSC_OB = OpenSCADObject in solidpython.py.

If it is a matter of circular imports you can use the TYPE_CHECKING flag in typing to only import things if it is not being executed. Also you can use string literals where need (i.e. ... -> "OpenSCADObject")

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/SolidCode/SolidPython/issues/116?email_source=notifications&email_token=AAEWGLMCMD7M6RSY25QPCLTQN6QN3A5CNFSM4I7RTPFKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEA6DMPY#issuecomment-540816959, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEWGLOJBKTMBRHPX3LAESLQN6QN3ANCNFSM4I7RTPFA.

etjones commented 4 years ago

Just pushed type hints to master & published v0.4.3 to PyPI.