Add ons manager - development and bugs topic

[vocx]

If you found something incredibly stupid in the code, please tell me, I will be happy to make it better. I have not too much experience with python idioms.

I think you should remove the comment lines and blocks that separate the functions, they are annoying and not needed. Whitespace is what will visually tell you when a block of code starts or ends.

I find strange your convention of variables in browser.py. Variables that start with an underscore are meant to be hidden, private to a single function or module. But in your case, you use them as global variables. And I don't get why you need them to be global. You should try to pass variables more, instead of having them global all over the place.

Code: Select all

_browser_instance = None
_browser_session = {}

Also, I wouldn't use Python f"strings" yet. This was introduced in Python 3.6, I think. But I remember some Linux system, like Ubuntu 16.04, still carry an old version like Python 3.5, so this feature is not available everywhere. I would wait at least a year or two before starting to use newer Python features like this.

Also, in general, using eval is bad. I would try to avoid it.

I strongly advice you to use Python's snake_case functions. It seems you used camelCase in many cases, but that is not Pythonic. Check PEP8. Use an editor like Spyder, which will immediately check your style while you type.

Also, don't import modules inside functions. Place the imports at the top of the document where it is easily recognizable what is imported. The first time a module is imported, it is added to the Python cache, so it won't be imported twice. Placing an import like FreeCADGui inside a function in controller.py doesn't make much sense because your tool is graphical, so it will be imported anyway. It should be at the top, where people can clearly see what is being called from elsewhere.

[mnesarco]

I think you should remove the comment lines and blocks that separate the functions, they are annoying and not needed. Whitespace is what will visually tell you when a block of code starts or ends.

Ok, I come from java wold where function comments are before function declaration and it makes the code very readable. But i understand python is another world. I find difficult to find functions without any separator, but will remove them just to follow the more pythonic way.

I find strange your convention of variables in browser.py. Variables that start with an underscore are meant to be hidden, private to a single function or module. But in your case, you use them as global variables. And I don't get why you need them to be global. You should try to pass variables more, instead of having them global all over the place.

Code: Select all

_browser_instance = None
_browser_session = {}

I do not use globals everywhere, they are static module private singletons. For example _browser_instance holds an instance of browser that must be unique in the App.

I use some constants in the module level, specially for the regular expression patterns, i do not want to recreate them each time.

Also, I wouldn't use Python f"strings" yet. This was introduced in Python 3.6, I think. But I remember some Linux system, like Ubuntu 16.04, still carry an old version like Python 3.5, so this feature is not available everywhere. I would wait at least a year or two before starting to use newer Python features like this.

Ok, i was not aware of that. I will change to legacy format.

Also, in general, using eval is bad. I would try to avoid it.

eval is used exclusively in the template engine, it is a powerful template engine done with very few code. it is used to generate html code with small python fragments embeded.

I strongly advice you to use Python's snake_case functions. It seems you used camelCase in many cases, but that is not Pythonic. Check PEP8. Use an editor like Spyder, which will immediately check your style while you type.

Ok, another culture clash from java/python. I will do a general review of the style and make it the python way.

Also, don't import modules inside functions. Place the imports at the top of the document where it is easily recognizable what is imported. The first time a module is imported, it is added to the Python cache, so it won't be imported twice. Placing an import like FreeCADGui inside a function in controller.py doesn't make much sense because your tool is graphical, so it will be imported anyway. It should be at the top, where people can clearly see what is being called from elsewhere.

As far as i understand, modules are not imported twice, so importing them sometimes inside functions is not a double import, in that case the import is a noop. Anyway i will check where it has non sense. Sometimes is better to import modules inside functions where the function is rarely called. In that case the modules are not imported unnecessarily when using the rest of the functions in the parent module.


Thanks for your extensive review.

[vocx]

As far as i understand, modules are not imported twice, so importing them sometimes inside functions is not a double import,...
In that case the modules are not imported unnecessarily when using the rest of the functions in the parent module.

It's not about importing twice, it's about readability. When I open a file, I can see immediately the imports, and get an idea of what will be called elsewhere in the code. However, if the imports are "hidden" inside a function, then we don't know what is required without going through the code. If you make a change and it breaks an import, the error won't be found until the user runs that particular function; in many cases that error can be spotted just by importing the faulty module at the top.

Also, don't worry too much about performance. Premature optimization is bad. Optimize later when you realize importing of the modules is very slow. Otherwise, it's not necessary.

It may be helpful to use the LazyLoader class introduced in the Path workbench. It seems to work for them, New python library for deferred loading.

[mnesarco]

I have fixed most python style issues, except for the camel Case to snake case because it is pretty difficult to do. Python refactoring is not as easy as java refactoring.

@vocx, I appreciate all your style corrections. I am new to python. I hope you do not dislike the project just for that small things.

[vocx]

... Python refactoring is not as easy as java refactoring.

I think you are mistaken. Python is easier than Java.

[kkremitzki]

Python in general being easier than Java is a point many would agree with, but in the particular case of refactoring I'm not so sure... static analysis and refactoring tools are a big part of the reason why people pick the language in the first place.

[mnesarco]

Hi Friends,

I will not discuss something like Java vs Python but I will try to make the code as pythonic as possible. But FreeCAD and PySide does use CamelCase and not snake_case.

I am and old guy, more than 20 years of java coding, so my brain is compiled in java . But I don't care to code in python, it is a good scripting language. I just try to contribute something to the FreeCAD Community, I don'ta want to change the rules of anything

Cheers,
Frank.

[mnesarco]

I have changed almost everything to snake_case except some core classes because:

1. Some of them are serialized/deserialized from external json files and there are some parts reading that json files and use the current attribute names (CamelCase). So it is hard to refactor because static analysis cannot identify that dynamic usages.

2. Html templates uses them in embedded python chunks but the IDE (PyCharm) does not recognize python inside html files. So that python blocks are invisible to static analysis/refactoring.

Most of the FreeCAD python API uses CamelCase, also PySide uses CamelCase, so I thing making everything snake_case here is not an uber strong requirement.

Now I will continue with functional use cases like installing through http without git.

[vocx]

...
Most of the FreeCAD python API uses CamelCase, also PySide uses CamelCase, so I thing making everything snake_case here is not an uber strong requirement
...

Changing the FreeCAD side of things could be done. It isn't going to happen in a day or month or year, but eventually it could be done. Like Blender, most of their commands use snake case.

As for PySide. It has been mentioned that the next version of Qt, that is, Qt6 will use a more Pythonic interface, and the same should be true for PySide6 (they will move the version from PySide2 to PySide6 to match the Qt version).

[sgrogan]

I hope you like it.

I was able to install the WB using Addons-manager. When activating the Extension Manager WB is empty.

Capture.PNG (61.58 KiB) Viewed 1187 times

OS: Windows 10 (10.0)
Word size of OS: 64-bit
Word size of FreeCAD: 64-bit
Version: 0.19.21189 (Git)
Build type: Release
Branch: master
Hash: 0d416c807bfb12e27806e375c432a3ed08ba8701
Python version: 3.6.8
Qt version: 5.12.1
Coin version: 4.0.0a
OCC version: 7.3.0
Locale: English/United States (en_US)