-
Notifications
You must be signed in to change notification settings - Fork 31
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
[DO NOT MERGE] importsimportsimports #467
base: frost
Are you sure you want to change the base?
Conversation
all routes get each other so feeds.py was importing jinja2. stop doing that.
to those onlooking (none of you), yes, changing the sort order of imports can break things |
seriously never import the app from a class model. if you need an environment variable, use files.helpers.config.environment
the state of affairs is that `g` was used in a lot of places because it is extremely convenient. it's pretty difficult to get them completely out of classes due to how deeply embedded it is in some parts (especially of the comments system). instead of trying to rework it (like i tried upstream to mild success), we'll just go for the consolation prize of narrowing import scope to `g`.
i don't know if this will help, but it might help for the get_post loop case
are we really getting much of a benefit from caching it anyway? performance input needed and maybe some hacky way of injecting this at runtime. the problem is cache causes import loops because it's one of those "imported from __main__" things.
In conclusion, get test code coverage to at least 70%+, including all routes, before touching this. |
agreed. part of the problem is that everything is so coupled that it's impossible to test only classes. like as mentioned before back when the leaderboard issue was discussed, classes will pull in this will hopefully let classes be tested (mostly) in isolation, only requiring getting proper environment variables. reference (cc: @TLSM): https://discord.com/channels/722487636488486913/970874103542218792/1044281306089001080 part of the issue was that classes end up, by proxy, pulling in routes, and then you got all of that mess where attempting to change the user class would cause deadlocks when attempting to run migrations |
perhaps a relatively ambitious thing and may never really happen but...
on rdrama me and @TLSM did importmania (see https://github.com/Aevann1/rDrama/pull/442), one of the most expansive reworks for rdrama's codebase, essentially organizing the codebase so that it doesn't depend on the app starting up just to import classes and other crap like that
for an example of the issues caused by this, see this note let in
files/classes/__init__.py
. it readsanyway this PR is essentially a diet version of importmania.
one thing to note though:
DO NOT MERGE THIS WITHOUT EXTENSIVE TESTING ON PRETTY MUCH ALL ROUTES
edit: it's worth noting, to the unfortunate souls that have the task of reviewing this, that these are not really meant to be taken commit by commit. while
tokencommit integrity is attempted to be preserved, in this case, it's not about the journey, but rather the destination