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

[DO NOT MERGE] importsimportsimports #467

Draft
wants to merge 65 commits into
base: frost
Choose a base branch
from

Conversation

justcool393
Copy link
Collaborator

@justcool393 justcool393 commented Jan 8, 2023

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 reads

################################################################
#                 WARNING! THIS FILE IS EVIL.                  #
################################################################
# Previously, this file had a series of                        #
#     from .alts import *                                      #
#     from .award import *                                     #
#     from .badges import *                                    #
# and so on in that fashion. That means that anywhere that     #
#     from files.classes import *                              #
# (and there were a lot of places like that) got anything      #
# was imported for any model imported. So if you, for example, #
# removed                                                      #
#     from secrets import token_hex                            #
# from files/classes/user.py, the registration page would      #
# break because files/routes/login.py did                      #
#     from files.classes import *                              #
# in order to get the token_hex function rather than           #
# importing it with something like                             #
#     from secrets import token_hex                            #
#                                                              #
# Anyway, not fixing that right now, but in order to           #
# what needed to be imported here such that                    #
#     from files.classes import *                              #
# still imported the same stuff as before I ran the following: #
#     $ find files/classes -type f -name '*.py' \              #
#         -exec grep import '{}' ';' \                         #
#         | grep 'import' \                                    #
#         | grep -v 'from [.]\|__init__\|from files.classes' \ #
#         | sed 's/^[^:]*://g' \                               #
#         | sort \                                             #
#         | uniq                                               #
# and then reordered the list such that import * did not stomp #
# over stuff that had been explicitly imported.                #
################################################################

anyway 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 token commit integrity is attempted to be preserved, in this case, it's not about the journey, but rather the destination

@justcool393 justcool393 added P4 long-term WIP Work In Progress and removed P4 long-term labels Jan 8, 2023
@justcool393
Copy link
Collaborator Author

justcool393 commented Jan 8, 2023

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.
@zorbathut
Copy link
Contributor

In conclusion, get test code coverage to at least 70%+, including all routes, before touching this.

@justcool393
Copy link
Collaborator Author

justcool393 commented Jan 9, 2023

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 __main__ which will pull in routes and pulling in routes has side effects for reasons that cannot be explained.

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

@justcool393 justcool393 mentioned this pull request Mar 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P4 long-term WIP Work In Progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants