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

Split utils.py into multiple coherent modules #1857

Open
MusicalNinjaDad opened this issue Jun 7, 2024 · 5 comments
Open

Split utils.py into multiple coherent modules #1857

MusicalNinjaDad opened this issue Jun 7, 2024 · 5 comments

Comments

@MusicalNinjaDad
Copy link
Contributor

#1851 introduced a change into utils.py which required an in-function import in order to avoid a circular import with logger.py

Originally posted by @henryiii in #1851 (comment)

We can probably split utils up later to avoid the circular import, it's pretty big. Not something to worry about for this PR.

Needs design input

@MusicalNinjaDad
Copy link
Contributor Author

MusicalNinjaDad commented Jun 7, 2024

I'd be happy to collaborate on this if it is something considered worth doing ... (please at-tag me to get my attention ;))

@henryiii
Copy link
Contributor

henryiii commented Jun 8, 2024

Sort of related: I'd also like to put the four platform files in a folder (macos, linux, windows, and pyodide).

@mayeut
Copy link
Member

mayeut commented Jun 8, 2024

I guess it can be done incrementally rather than all at once.
So I'd say for util.py, it could be made as a folder, rename the file as misc.py in that folder and then start splitting this up.
If trying to break-up by family, I'm seeing (not exhaustive at all, to be extended):

  • util/provider.py to provide CI provider stuff (it's the one causing the circular import if I'm not mistaken)
  • util/file.py for all file manipulation (download, extract_*, move_file, maybe chdir, ...)
  • util/tool.py for virtualenv, node, uv stuff.
  • util/string.py for string manipulation (unwrap, strtobool, ...)

@MusicalNinjaDad
Copy link
Contributor Author

Hi both,
I see that you've suggested to remove the logging from move_file, mayeut, so this potentially loses importance.(?)
But it's probably still a nice clean up, which I'd be happy to help with if you agree it is worthwhile.

I've tried a quick check - moving everything to util/misc.py leads to plenty of import errors found by pylint. Adding a from .misc import * to util/__init__.py then fixes all those errors - so that appears a viable approach to allow for an incremental move.

I'll take a good look through util.py and maybe try moving out the first family as an initial PR ...

@MusicalNinjaDad
Copy link
Contributor Author

I've made a suggestion of how we could approach this with the above PR. Would be interested in feedback from both of you on your thoughts ...

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

No branches or pull requests

3 participants