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

fix/refactor: add missing imports and reorganize some alls #1672

Open
wants to merge 2 commits into
base: unstable
Choose a base branch
from

Conversation

AstreaTSS
Copy link
Member

@AstreaTSS AstreaTSS commented Apr 30, 2024

Pull Request Type

  • Feature addition
  • Bugfix
  • Documentation update
  • Code refactor
  • Tests improvement
  • CI/CD pipeline enhancement
  • Other: [Replace with a description]

Description

I've recently developed a tool that goes in and makes sure that each __init__.py matches up with the __all__s in files/folders in the same directory as it. For example, if there was a statement like from .channel import GuildText, the program will go in and make sure the __all__ of channel is ("GuildText",), and fixes the import if the __all__ doesn't match up. It also detects if the __init__ isn't importing a file with a __all__ in it and adds it to the import.

This tool is far from ready, but I've gotten it to a state where I could run a run on the interactions.py library. Wouldn't you know it, we are missing some imports, so I made this PR to add them back in. This PR also fixes some other issues I was running into involving the codebase, like GuildMedia not being properly exposed in channel.py.

As a (hopefully good) consequence of this program, __all__ for __init__.pys that have been affected have been put in alphabetical order.

Changes

  • Add GuildMedia to __all__ in channel.py for models.
  • Export CronTrigger.
  • Export LocalisedField and LocalizedField.
  • Export ClientObject and DiscordObject. You may argue against it, but I'd argue that there's no good reason to not export them, and they can have their uses.
  • Oh right, this program also picked up deserialize_app_cmds, interestingly enough.
  • Do an alphabetical sort for all __all__s that have been adjusted.

Related Issues

Test Scenarios

Python Compatibility

  • I've ensured my code works on Python 3.10.x
  • I've ensured my code works on Python 3.11.x

Checklist

  • I've run the pre-commit code linter over all edited files
  • I've tested my changes on supported Python versions
  • I've added tests for my code, if applicable
  • I've updated / added documentation, where applicable

@AstreaTSS AstreaTSS mentioned this pull request May 6, 2024
13 tasks
@LordOfPolls
Copy link
Contributor

Your code sounds cool, but out of interest how are you validating for objects that should not be exposed to the top level API of a library? Its not always desirable to expose everything under the root namespace.

Or is this just a expose-everything situation?

@AstreaTSS
Copy link
Member Author

Your code sounds cool, but out of interest how are you validating for objects that should not be exposed to the top level API of a library? Its not always desirable to expose everything under the root namespace.

Or is this just a expose-everything situation?

Sorry, couldn't look at the code I made for a while 😅. Anyways, the code goes in and exposes everything in a file's __all__, so it's not the most robust thing in the world. Hence why the PR here was limited to only running in places I deemed "safe", and even then, in retrospect, it may have been too zealous.

I'm not sure if there's a way of making some objects not exposed to the top level without adding it to some ignore option or the like to the script.

@AstreaTSS
Copy link
Member Author

For transparency's sake, since it's been some months and this PR would have to be recreated if we want to continue with it, here's the script that generated everything: https://gist.github.com/AstreaTSS/9032753c1ae1b8ece431a87d31ca166f

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