-
Notifications
You must be signed in to change notification settings - Fork 147
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
Move manage.py
content to evap.__main__
#2387
Conversation
This allows running management commands with `python -m evap` if evap is installed into the environment.
manage.py
Outdated
|
||
settings.DATADIR.mkdir(exist_ok=True) | ||
execute_from_command_line(sys.argv) | ||
main() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not that anyone would ever import this file, but is there a specific reason to remove the if __name__ == "__main__"
guard?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No reason except brevity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can add it back if you want
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean ideally, this would be python -m evap
directly... What about https://docs.python.org/3/library/runpy.html#runpy.run_module?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
runpy
sure is ... something :D It fixes the missing coverage, but it adds some complexity that we might not want. @richardebeling ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the right thing to do to me. I'm not sure I get your points about missing coverage (you mean test coverage? Would that have been a problem?) and complexity (import runpy and runpy.run_module look canonical to me, I wouldn't care what they do under the hood -- do I need to care?), unless I'm missing something here I'm very fine with the current code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering whether runpy
magic could break something like ./manage.py test --pdb
, but it seems to work just fine. My points are
- without runpy, the line in
__main__.py
that callsmain
is not covered, but with runpy, it is (although the branch where__name__
is not__main__
is uncovered now :D) - I also don't care what runpy does and don't want to care, I was just afraid that we would have to care some day. It also makes the
manage.py
file less approachable to first semester students, but I guess they have to accept our arcane magic anyways once they hitexecute_from_command_line
I also think runpy is fun, so we can keep it if the above don't concern you :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, makes sense. I'm fine with both approaches.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think both runpy or not is fine
I noticed that the usage text is sometimes messed up, for example |
This is in preparation for #2328, where we want to include the
manage.py
code, but cannot include themanage.py
file because it is outside theevap
module. Using__main__
allows running management commands withpython -m evap
if evap is installed into the environment. Runningmanage.py
still works, as it uses the builtinrunpy
module to simulate runningpython -m evap
.