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

Draft: split objects and functions from Draft.py #3388

Merged
merged 15 commits into from Apr 29, 2020

Conversation

carlopav
Copy link
Contributor

Add DraftObject and ViewProviderDraft to serve as
the parent classes of all Draft objects
and all Draft view providers.

Inside Draft.py we need to import
_DraftObject from draftobjects.base;
and _ViewProviderDraft, _ViewProviderDraftAlt,
and _ViewProviderDraftPart from draftviewproviders.view_base.

@carlopav
Copy link
Contributor Author

@vocx-fc any hints why it fails with py2? I do not spot any error related to Draft in the build log...

@vocx-fc
Copy link
Contributor

vocx-fc commented Apr 25, 2020

Oh, Bernd has been doing some edits to FEM, and now it's causing errors with Python 2. So, yeah, it's not related to Draft.

@carlopav
Copy link
Contributor Author

ok, so let's go on with cherrypicking old commits to make the pr ready to merge!

@carlopav
Copy link
Contributor Author

Here we are. This was the easy part. We can now move to another PR to tackle the functions before going back to arrays objects.

@carlopav
Copy link
Contributor Author

@vocx-fc @yorikvanhavre there is one thing that I do not like much: all the make functions are renamed make_something. I actually preferred the old makeSomeThing since SomeThing is the exact name of the object.
Can we change that back?

@vocx-fc
Copy link
Contributor

vocx-fc commented Apr 25, 2020

Awesome. As I can see, you aren't using my commit any more, right? Now it's all authored by yourself. Yes, I think this is the way to go. We don't have to move all objects in a single super huge pull request, we can move a few at a time, so it's easier to review and spot problems.

@carlopav
Copy link
Contributor Author

carlopav commented Apr 25, 2020

Well I thought It was better to start from scratch from current upstream master.
Anyway the base classes are the ones of your initial commit.
What about the makeSomeThing approach? I think it would help all the other modules that use those functions outside Draft to not have them duplicated.

@vocx-fc
Copy link
Contributor

vocx-fc commented Apr 25, 2020

I'm not sure what the problem is. The make_something is PEP8 compliant, and we want it to be compliant. We should provide the makeSomething style to keep compatibility but in the future, when we are ready to break compatibility, we can say, "guys, makeSomething is deprecated, use make_somthing from now now", and we should change the calls.

But you don't need to duplicate it. Python is flexible, so you can rename a function to any name we want. This can be done at import time.

from draftmake.make_circle import make_circle
from draftmake.make_circle import make_circle as makeCircle

The name of the Proxy class is the important thing and must be preserved, but the make function can be anything.

@carlopav
Copy link
Contributor Author

I like that style of import! still I have this doubt about being PEP8 compliant for those functions that are at the moment spread around the whole FreeCAD modules, macros etc... (objects are not, makefunctions are).

PR: #3394 follows this one!

@vocx-fc
Copy link
Contributor

vocx-fc commented Apr 26, 2020

Well, my grand idea is that we should make the entire FreeCAD PEP8 compliant, but I'm only a single person, so I can only change things little by little. For example, FEM is pretty much PEP8 compliant because Bernd has kept it that way. On the other hand, we may have to talk to the Path guys because they don't seem to be compliant either. Making sure the big Python workbenches follow PEP8 would be great but it is hard work.

@carlopav
Copy link
Contributor Author

carlopav commented Apr 27, 2020

@vocx-fc ok, at the moment it's like this, so let's keep it PEP8!
@yorikvanhavre if you want this and the following branches are ready to be merged (I will not add more commits to them). Py2 build failure is not related to Draft.

@yorikvanhavre
Copy link
Member

Sorry I merged one other PR in the wrong order... Would you mind having a look at the conflict here @carlopav ?

@kryptokommunist
Copy link
Contributor

kryptokommunist commented Apr 27, 2020

The conflict stems from my modification of Draft.py here: #3387 . The original idea would have been for me to integrate it into @carlopav's changes, but the other way around should be alright too since this is not a big change.

There should be an automated way to specify dependencies between pull requests by the contributors...

Add DraftObject and ViewProviderDraft to serve as
the parent classes of all Draft objects
and all Draft view providers.

Inside `Draft.py` we need to import
`_DraftObject` from `draftobjects.base`;
and `_ViewProviderDraft`, `_ViewProviderDraftAlt`,
and `_ViewProviderDraftPart` from `draftviewproviders.view_base`.
.


.


.


.
Line Polyline BezCurve BSpline
.


.


.
@carlopav
Copy link
Contributor Author

@kryptokommunist Do not worry,the array classes are not splitted yet, so no problem :)

@carlopav
Copy link
Contributor Author

@yorikvanhavre rebased and ready for merging :)

@yorikvanhavre
Copy link
Member

Okay, thanks!! Waiting for the CI test to finish and I merge

@carlopav
Copy link
Contributor Author

@yorikvanhavre good! mind that the test fail with py2 as every other PR in the last week :)

@yorikvanhavre yorikvanhavre merged commit 582a4c0 into FreeCAD:master Apr 29, 2020
@carlopav
Copy link
Contributor Author

uuuuuuuu..... scaring :)

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

4 participants