Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cleanup and organize utils #7033

Merged
merged 10 commits into from
Jul 6, 2023
Merged

Cleanup and organize utils #7033

merged 10 commits into from
Jul 6, 2023

Conversation

NickM-27
Copy link
Sponsor Collaborator

@NickM-27 NickM-27 commented Jul 4, 2023

The util class was getting a bit unwieldy, I think it is good to organize the utils at this point

@netlify
Copy link

netlify bot commented Jul 4, 2023

Deploy Preview for frigate-docs ready!

Name Link
🔨 Latest commit 9779206
🔍 Latest deploy log https://app.netlify.com/sites/frigate-docs/deploys/64a6c44450491c0008de624a
😎 Deploy Preview https://deploy-preview-7033--frigate-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@skrashevich
Copy link
Contributor

maybe, simplify this by use https://github.com/scientific-python/lazy_loader ?

@NickM-27
Copy link
Sponsor Collaborator Author

NickM-27 commented Jul 5, 2023

maybe, simplify this by use https://github.com/scientific-python/lazy_loader ?

Thanks, that is cleaner and eliminates needing to guess which sub module something is in

actually, this doesn't work the way I thought it did. And as far as I can tell it breaks the code completion / type checking in vscode

@skrashevich
Copy link
Contributor

actually, this doesn't work the way I thought it did. And as far as I can tell it breaks the code completion / type checking in vscode

can you provide an example?

@NickM-27
Copy link
Sponsor Collaborator Author

NickM-27 commented Jul 5, 2023

actually, this doesn't work the way I thought it did. And as far as I can tell it breaks the code completion / type checking in vscode

can you provide an example?

As this PR is now vscode recognizes the function and ctrl + clicking it opens that function, also will warn if incorrect parameters are supplied to the function

Screen Shot 2023-07-05 at 16 13 28 PM

When trying to use lazy loading based on their readme, it does not recognize the functions.

import frigate.util as utils

Screen Shot 2023-07-05 at 16 14 39 PM

It is possible it could be some cache thing or something but I did restart vscode and also ran frigate. Frigate worked as expected but the issue above remained.

@skrashevich
Copy link
Contributor

Are you sure you didn't forget to create appropriate .pyi files?

@NickM-27
Copy link
Sponsor Collaborator Author

NickM-27 commented Jul 6, 2023

Are you sure you didn't forget to create appropriate .pyi files?

Ah, I saw type checkers but figured that would just be for mypy, I now see that it also applies to IDEs which makes sense. I will try this later and I expect that it will most likely solve the issue

@NickM-27
Copy link
Sponsor Collaborator Author

NickM-27 commented Jul 6, 2023

Okay it is working now but I am unsure this really makes things any simpler, it will now require adding each function that is added to any util in the __init__.pyi file:

__all__ = ['EventsPerSecond', ' clean_camera_user_pass', ' deep_merge', ' escape_special_characters', ' get_ffmpeg_arg_list', ' get_tz_modifiers', ' load_config_with_no_duplicates', ' load_labels', ' to_relative_box']
from .builtin import EventsPerSecond, clean_camera_user_pass, deep_merge, escape_special_characters, get_ffmpeg_arg_list, get_tz_modifiers, load_config_with_no_duplicates, load_labels, to_relative_box

and you will still have to use utils.builtin.to_relative_box() for example which still requires knowledge of which module the function is in.

I think something like this may be worth doing in the future but this PR is meant to be a simple reorganization effort

there will already be benefit to improved importing since the entire set of utils no longer need to be imported

@skrashevich
Copy link
Contributor

maybe, something like:

import inspect
import .builtin as builtin_module

__all__ = [name for name, obj in inspect.getmembers(builtin_module) if not name.startswith('_') and (inspect.isfunction(obj) or inspect.isclass(obj))]

@blakeblackshear
Copy link
Owner

Yea. I don't see enough of a benefit for automatic imports. Breaks the navigation in github too.

Just a few conflicts to resolve now.

@blakeblackshear blakeblackshear merged commit baf671b into dev Jul 6, 2023
10 checks passed
@blakeblackshear blakeblackshear deleted the util-cleanup branch July 6, 2023 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants