Minor refactoring of get_object_by_role() in HardwareObjectNode class #855
Conversation
mxcubecore/BaseHardwareObjects.py
Outdated
| for object in obj: | ||
| objects.append(object) | ||
| try : | ||
| self = objects.pop() |
There was a problem hiding this comment.
Reassigning self to another value is really confusing (and arguably unpythonic).
Also the combination of making and popping lists confuses at least me (though it was already there in the previous version), so I might be misunderstanding what this does, but assuming I got it right, I would propose the following code as a better simplification. This does breadth-first traversal of the tree of objects in a single loop without recursive calls
role = str(role).lower()
objects = [self]
for curr in objects:
result = curr._objects_by_role.get(role)
if result is None:
objects.extend(obj for obj in curr if obj)
else:
return result
Of course the thing we most need to do with HardwareObjectNode is to replace it with yaml-configured objects, so I would not do any more work than necessary on cleaning it up.
There was a problem hiding this comment.
Excellent! Thank you, that is indeed more elegant. I tested it and didn't notice any issues. I commited your recomanded changes.
|
Hi @HilbertCorsair, |
Remove package ispyb-client from mxcubecore's dependencies. It is not used anymore, and also it is causing issues due to it's 'pinned' dependency to typing_extensions package at version 4.3.0.
Remove formatting and linting dependencies from the conda environment since they are in pre-commit already. Remove test dependencies from the conda environment since they are in the Poetry dev dependencies already. Remove code related to `importlib_metadata` on Python "<3.8". Remove dependency version constraints for Python "<3.8" since we only support Python ">=3.8,<3.11". Remove setuptools and setuptools-scm from build dependencies since they are superfluous.
Use the `--force` flag of `mamba env create` in case environment exists. Skip `poetry run` when running `sphinx-build` since Poetry installs in the conda environment directly. Remove the workaround for the unwarranted error about "missing `conda.environment`" configuration setting, because it has been fixed already. See: * readthedocs/readthedocs.org#10979 (comment) * readthedocs/readthedocs.org#11040
Remove hardware objects that are no longer used at MAXIV. These objects are not maintained anymore, so let's remove them to cut down on possible future confusion.
Fix the pinned version for `python-ldap`. Add some explanatory comments. Remove `pip` since it is already a dependency of `python`. Remove `openldap` since it is already a dependency of `python-ldap`.
…ction. Updated documentation to match
HilbertCorsair
force-pushed
the
develop
branch
from
5682524 to
4e31839
Compare
|
You're right @beteva sory about that. I did the rebase. |
There was a problem hiding this comment.
Something is really weird here. The PR now registers as having 75 files changed instead of 3. If you look at the actual changes (most have to do with GPhL files) it looks like the red code (supposedly the previous state) is not the code that is in there now. The green code looks to a quick look like it is what is there already, which should be correct. Before we merge this, can someone explain what is going on?
|
When I pushed the changes after the rebase I used the --force-with-lease option so all the devellopments that were made on the upstream are kept. The 2 commits related to my refatoring are included with all the other commits made in the meantime by other developers. I think that's why all those other files show up. |
|
I ran into this method while trying to find a silent error that was causing the app to fail. It turns out it was a JavaScript error on the front end having to do with detector configuration issues. Nevertheless, I thought it would be a good idea to propose a quick refactoring and eliminate unnecessary looping . |
|
As Antonia said - yes, this is a good initiative. Don't let me discourage you ;-) On the rebasing - I tested a bit locally, and I think you should be able to get the kind of tree you want if you make a new branch as a copy of upstream develop, and cherrypick the commits you want to it. What you have now looks like you are recommitting a lot of commits that are already in the develop branch, and even if it could give the right final result, it would be a horribly complex commit history. Could you give it a try? |
|
Yes, that sounds like a better idea. Just to make sure I understand, I wil have to close this current PR and create a new pull request from the new branch right ? |
|
This refactoring will be submitted from a new branch. |
There are probably many aspects that can be improved in the HardwareObjectNode class, particularly in error handling and logging. For now, I just made some minor changes to the get_object_by_role() method.
These changes eliminate unnecessary looping, they also eliminate the infinite loop and hopefully makes the core prettier.
I really hope I didn't miss anything. Please let me know what you think?