search

mxcube / mxcubecore

Backend used by MXCuBE
http://mxcube.github.io/mxcube/
GNU Lesser General Public License v3.0
11 stars 51 forks source link

Minor refactoring of get_object_by_role() in HardwareObjectNode class #855

Closed HilbertCorsair closed 4 months ago

HilbertCorsair commented 4 months ago

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?

beteva commented 4 months ago

Hi @HilbertCorsair, cleaning the code is a a good initiative, I guess, though, having the TangoShutter HO and test here is by mistake (maybe a rebase is missing). Could you, please, remove it. Thank you

HilbertCorsair commented 4 months ago

You're right @beteva sory about that. I did the rebase.

HilbertCorsair commented 4 months ago

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. However I would fill better is someone checks if this is ok. I made these last changes on the same local develop branch as I did last time for the TangoShutter. Now that I did a rebase and removed the previous commits I'm not sure the previous changes to the TangoShutter class are still kept anywhere on the upstream. I made a backup of the branch before the rebase just in case.

HilbertCorsair commented 4 months ago

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 .

rhfogh commented 4 months ago

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?

HilbertCorsair commented 4 months ago

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 ?

HilbertCorsair commented 4 months ago

This refactoring will be submitted from a new branch.