The script can be used to create
- test and expected objects with the given names
- generate vertex groups based on the vertex index specified
Details
Details
- Reviewers
Habib Gahbiche (zazizizou)
Diff Detail
Diff Detail
Event Timeline
Comment Actions
Coding style:
- Have a look at Blender's coding style and revise your code
- No documentation and 0 comments is usually bad. A basic doc string per function would really help understand what you are trying to achieve and how users are supposed to use your script.
General approach:
- Why not merge creating a vertex group and creating a test object in a single function?
- We discussed the option to create a random vertex group. Any plans to add that?
- I'm not strongly against global variables but here it's probably best to create a class that stores all internal parameters/states in one place. So a bit similar to 'MeshTest', where you have a class that does the actual work and another class that stores specifications.
| tests/python/modules/test_object_generator.py | ||
|---|---|---|
| 16–18 | This is redundant. You already have max value so just check if max is larger than the allowed maximum. You can do the same for negative values with min(..) | |
| 21 | the function is called do_selection but it also assigns vertex group? You should avoid this for two reasons:
| |
| 35 | what if the object does not exist? | |
| 40 | You should validate function input before doing anything, otherwise the function exists with an exception with the job half way done. You also probably don't need an extra function to validate input. Just write the validation code at the beginning of this function | |
| 57–62 | Not sure why you need a function here, you are wrapping only 1 line? | |
| 77–79 | You don't have to duplicate this code for every obj_type, just write it once after all the if statments | |
| 140–147 | This has to be removed. The script must have a well defined list of arguments. The user as well as the underlying function can 'agree' on what the arguments mean beforehand. Ideally you should print a help message about how the script is supposed to be used, what it does and so on... | |
| 157 | The file will get created even when there are errors later on. Is this intentional? It's probably best to either create a file that has been generated correctly or don't create anything at all, right? | |
Comment Actions
I want to suggest to move on as soon as the script does what it's supposed to do in a correct and stable manner. Otherwise we could be improving the script forever without gaining much.
| tests/python/modules/test_object_generator.py | ||
|---|---|---|
| 33 | It looks like you are passing an object and not a mesh, so argument name seems wrong... | |
| 38 | Use True/False instead | |
| 140 | This is one more reason why a class would be better here, where you save all internal states within the class instead of having to declare global variables. Also, flag is not descriptive enough. Something like success is more commonly used | |
| 161 | is conversion to str really necessary? I thought it could work without it.. | |
| 171–172 | Does this really work? flag is set inside the function create_test_objects() but it only gets checked before that function is called... | |
| tests/python/modules/test_object_generator.py | ||
|---|---|---|
| 33 | You were right I think, object is a keyword in Python and it's probably best to choose something else. | |