Review Blender ID addon #48024

New Issue

Sybren A. Stüvel · 2016-04-01T11:13:40+02:00

Sybren A. Stüvel commented

2016-04-01 11:13:40 +02:00

Hey Campbell,

Can you review the Blender ID addon for inclusion in master? The README.md should contain everything needed to build, install and use the addon. I figured that to include the addon in master, we'd build it with python setup.py bdist and then unzip the resulting zip file into Blender's addon dir.

This addon is used by the Blender Cloud addon, which we'll offer for review soon too.

Thanks!

Hey Campbell, Can you review the [Blender ID addon](https://developer.blender.org/diffusion/BIA/) for inclusion in master? The README.md should contain everything needed to build, install and use the addon. I figured that to include the addon in master, we'd build it with `python setup.py bdist` and then unzip the resulting zip file into Blender's addon dir. This addon is used by the [Blender Cloud addon](https://developer.blender.org/diffusion/BCA/), which we'll offer for review soon too. Thanks!

Sybren A. Stüvel commented

2016-04-01 11:13:40 +02:00

Changed status to: 'Open'

Campbell Barton was assigned by Sybren A. Stüvel

2016-04-01 11:13:40 +02:00

Sybren A. Stüvel commented

2016-04-01 11:13:40 +02:00

Added subscriber: @dr.sybren

Inês Almeida commented

2016-04-01 11:24:43 +02:00

Added subscriber: @brita

Campbell Barton commented

2016-04-01 11:33:34 +02:00

Initial review:

imports should be moved inline (especially heavy modules like requests), so its not slowing down startup:
files shouldn't be created when loading the module, better do this on register instead. profiles_path = bpy.utils.user_resource('CONFIG', "blender_id", create=True)
Suggest to use __all__ to all py files to show whats meant for use outside the modules own code.
Probably you prefer to keep this as-is, but did you consider using a single operator for BlenderIdLogin, BlenderIdValidate, BlenderIdLogout?... if there are 2+ more similar operators likely to be added which also do a single operation. They could be split into regular functions (so code remains separated). Then a single operator can call any of them based on a single enum property.
Would be nice to have a template addon that uses this, some hello-world single file example.

Initial review: - imports should be moved inline (especially heavy modules like requests), so its not slowing down startup: - files shouldn't be created when loading the module, better do this on register instead. `profiles_path = bpy.utils.user_resource('CONFIG', "blender_id", create=True)` - Suggest to use `__all__` to all py files to show whats meant for use outside the modules own code. - Probably you prefer to keep this as-is, but did you consider using a single operator for `BlenderIdLogin`, `BlenderIdValidate`, `BlenderIdLogout`?... if there are 2+ more similar operators likely to be added which also do a single operation. They could be split into regular functions (so code remains separated). Then a single operator can call any of them based on a single enum property. - Would be nice to have a template addon that uses this, some hello-world single file example.

Sybren A. Stüvel commented

2016-04-01 14:25:33 +02:00

In #48024#367652, @ideasman42 wrote:

imports should be moved inline (especially heavy modules like requests), so its not slowing down startup:

I've moved requests and modules that are only used in one or two spots to inline. Commonly used modules (os, sys, json, bpy) have been kept at module level. I've also added support for reloading the addon. infrastructure/blender-id-addon@7721c3ff

files shouldn't be created when loading the module, better do this on register instead. profiles_path = bpy.utils.user_resource('CONFIG', "blender_id", create=True)

Done in infrastructure/blender-id-addon@fc139e5bc3

Suggest to use __all__ to all py files to show whats meant for use outside the modules own code.

Done in infrastructure/blender-id-addon@806f0c76

Probably you prefer to keep this as-is, but did you consider using a single operator for BlenderIdLogin, BlenderIdValidate, BlenderIdLogout?... if there are 2+ more similar operators likely to be added which also do a single operation. They could be split into regular functions (so code remains separated). Then a single operator can call any of them based on a single enum property.

We feel that the current approach (3 operators) is clearer for the user, as login/logout/validate are different actions, and not three variations of the same action.

Would be nice to have a template addon that uses this, some hello-world single file example.

We can add that to the README.md.

> In #48024#367652, @ideasman42 wrote: > - imports should be moved inline (especially heavy modules like requests), so its not slowing down startup: I've moved requests and modules that are only used in one or two spots to inline. Commonly used modules (os, sys, json, bpy) have been kept at module level. I've also added support for reloading the addon. infrastructure/blender-id-addon@7721c3ff > - files shouldn't be created when loading the module, better do this on register instead. `profiles_path = bpy.utils.user_resource('CONFIG', "blender_id", create=True)` Done in infrastructure/blender-id-addon@fc139e5bc3 > - Suggest to use `__all__` to all py files to show whats meant for use outside the modules own code. Done in infrastructure/blender-id-addon@806f0c76 > - Probably you prefer to keep this as-is, but did you consider using a single operator for `BlenderIdLogin`, `BlenderIdValidate`, `BlenderIdLogout`?... if there are 2+ more similar operators likely to be added which also do a single operation. They could be split into regular functions (so code remains separated). Then a single operator can call any of them based on a single enum property. We feel that the current approach (3 operators) is clearer for the user, as login/logout/validate are different actions, and not three variations of the same action. > - Would be nice to have a template addon that uses this, some hello-world single file example. We can add that to the README.md.

Sybren A. Stüvel commented

2016-04-01 16:02:27 +02:00

We're no longer using RNA to store authentication info; see infrastructure/blender-id-addon@4b43b5d53d

Campbell Barton commented

2016-04-02 11:16:37 +02:00

Great, some other points...

Regarding having 3x operations, its OK, just wanting to avoid having every single transaction with the server adding a new operator (in the case more are needed).
The point isn't so much that there is some fixed limit to the number of operators you should define, more that operators are intended for users, not so much for defining lower level API's.

Looking into this, the blender-cloud add-on isn't even calling the operators.
So perhaps these could be removed and kept as API's the Python module exposes.

There are also some minor changes I'd suggest, made a small patch. P345

Summary

Enforce utf-8 file io (which is json convention too), and not always assured to be default depending on the platform and Python env vars.
When there is a choice or it doesn't matter - use immutable data (tuples instead of lists).
Tweak style for property definitions (used all over existing code).

One other thing is this mixes single double quotes. Our existing convention is to use single for API identifiers/enums (what would be defines or enums in C/C++), and double-quotes for text content. No big deal but prefer this over mixing them randomly.

eg:

     blender_id_password = StringProperty(
            name="Password",
            default="",
            options={'HIDDEN', 'SKIP_SAVE'},
            subtype='PASSWORD',
            )

Great, some other points... Regarding having 3x operations, its OK, just wanting to avoid having every single transaction with the server adding a new operator (in the case more are needed). The point isn't so much that there is some fixed limit to the number of operators you should define, more that operators are intended for users, not so much for defining lower level API's. Looking into this, the blender-cloud add-on isn't even calling the operators. So perhaps these could be removed and kept as API's the Python module exposes. ---- There are also some minor changes I'd suggest, made a small patch. [P345](https://archive.blender.org/developer/P345.txt) Summary - Enforce utf-8 file io (which is json convention too), and not always assured to be default depending on the platform and Python env vars. - When there is a choice or it doesn't matter - use immutable data (tuples instead of lists). - Tweak style for property definitions (used all over existing code). One other thing is this mixes single double quotes. Our existing convention is to use single for API identifiers/enums *(what would be defines or enums in C/C++)*, and double-quotes for text content. No big deal but prefer this over mixing them randomly. eg: ``` blender_id_password = StringProperty( name="Password", default="", options={'HIDDEN', 'SKIP_SAVE'}, subtype='PASSWORD', ) ```

Sybren A. Stüvel commented

2016-04-03 11:47:18 +02:00

In #48024#367894, @ideasman42 wrote:
Regarding having 3x operations, its OK, just wanting to avoid having every single transaction with the server adding a new operator (in the case more are needed).
The point isn't so much that there is some fixed limit to the number of operators you should define, more that operators are intended for users, not so much for defining lower level API's.

Looking into this, the blender-cloud add-on isn't even calling the operators.
So perhaps these could be removed and kept as API's the Python module exposes.

I don't really understand what you mean with "these could be removed and kept". The operators are needed for the GUI login/logout/verify buttons.

Enforce utf-8 file io (which is json convention too), and not always assured to be default depending on the platform and Python env vars.

Good.

When there is a choice or it doesn't matter - use immutable data (tuples instead of lists).

Sure.

Tweak style for property definitions (used all over existing code).

This one is a bit annoying. All over the existing code PEP8 is declared at the top, which states "Hanging indents should add a level", and not two levels. PyCharm, the editor both Francesco and I use, has functionality for automatically formatting a file according to PEP8, which we use a lot. What is the reason that for RNA properties PEP8 is not followed?

One other thing is this mixes single double quotes. Our existing convention is to use single for API identifiers/enums (what would be defines or enums in C/C++), and double-quotes for text content. No big deal but prefer this over mixing them randomly.

I'm not a fan of adding semantics to the different quote styles. I do agree that consistency is nice, so I'll change them to single quotes, unless a single quote is part of the quoted string. This is also the behaviour of Python itself when printing repr(string), and consistent with PEP8.

> In #48024#367894, @ideasman42 wrote: > Regarding having 3x operations, its OK, just wanting to avoid having every single transaction with the server adding a new operator (in the case more are needed). > The point isn't so much that there is some fixed limit to the number of operators you should define, more that operators are intended for users, not so much for defining lower level API's. > > Looking into this, the blender-cloud add-on isn't even calling the operators. > So perhaps these could be removed and kept as API's the Python module exposes. I don't really understand what you mean with "these could be removed and kept". The operators are needed for the GUI login/logout/verify buttons. > - Enforce utf-8 file io (which is json convention too), and not always assured to be default depending on the platform and Python env vars. Good. > - When there is a choice or it doesn't matter - use immutable data (tuples instead of lists). Sure. > - Tweak style for property definitions (used all over existing code). This one is a bit annoying. All over the existing code PEP8 is declared at the top, which states "Hanging indents should add a level", and not two levels. PyCharm, the editor both Francesco and I use, has functionality for automatically formatting a file according to PEP8, which we use a lot. What is the reason that for RNA properties PEP8 is not followed? > One other thing is this mixes single double quotes. Our existing convention is to use single for API identifiers/enums *(what would be defines or enums in C/C++)*, and double-quotes for text content. No big deal but prefer this over mixing them randomly. I'm not a fan of adding semantics to the different quote styles. I do agree that consistency is nice, so I'll change them to single quotes, unless a single quote is part of the quoted string. This is also the behaviour of Python itself when printing repr(string), and consistent with PEP8.

Campbell Barton commented

2016-04-03 17:01:38 +02:00

Quick reply to some of these issues...

Grepped Py source for blender-cloud for "\.logout" & _OT_logout and didn't see any results. if they're needed, fine to keep of course.

Regarding "Hanging indents should add a level", this is indeed annoying. Note that when the code was written none of the error checkers reported on this (pep8/flake8/pyflakes/pylint ~ one of these is newer and wasn't around at the time but cant recall which :)).
The warnings were made more strict later (and quieting them would cause a lot of minor changes all over).
Having said that, checked the changes in my patch and they aren't causing these warnings for flake8.

So fine to leave as-is, just noting on my own preference/conventions, which in this case afaics don't conflict with pep8.

Quick reply to some of these issues... Grepped Py source for blender-cloud for `"\.logout"` & `_OT_logout` and didn't see any results. if they're needed, fine to keep of course. Regarding `"Hanging indents should add a level"`, this is indeed annoying. Note that when the code was written none of the error checkers reported on this (pep8/flake8/pyflakes/pylint ~ *one of these is newer and wasn't around at the time but cant recall which :)*). The warnings were made more strict later (and quieting them would cause a lot of minor changes all over). Having said that, checked the changes in my patch and they aren't causing these warnings for flake8. So fine to leave as-is, just noting on my own preference/conventions, which in this case afaics don't conflict with pep8.

Francesco Siddi commented

2016-07-29 13:02:40 +02:00

Added subscriber: @fsiddi

Francesco Siddi commented

2016-07-29 13:02:40 +02:00

Hi there @ideasman42!

We would like to proceed with the inclusion of the add-on as "official" in the upcoming Blender release. As originally requested, there is now a very clear use-case for it.

Let us know if there is anything else we can do.

Hi there @ideasman42! We would like to proceed with the inclusion of the add-on as "official" in the upcoming Blender release. As originally requested, there is now a [very clear use-case ](https://cloud.blender.org/services) for it. Let us know if there is anything else we can do.

Francesco Siddi commented

2016-08-09 10:57:56 +02:00

Changed status from 'Open' to: 'Resolved'

Francesco Siddi closed this issue

2016-08-09 10:57:56 +02:00

Francesco Siddi commented

2016-08-09 10:57:56 +02:00

Add-on has been merged!

Sign in to join this conversation.