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

The --pythonpath CLI option currently does nothing! #10296

Closed
rtibbles opened this issue Mar 22, 2023 · 4 comments · Fixed by #12874
Closed

The --pythonpath CLI option currently does nothing! #10296

rtibbles opened this issue Mar 22, 2023 · 4 comments · Fixed by #12874
Assignees
Labels
bug Behavior is wrong or broken

Comments

@rtibbles
Copy link
Member

This has probably been extant for quite some time, and probably got lost in a CLI refactor/reshuffle at some point.

The --pythonpath option should validate the file path, and append the path to the PYTHONPATH.

@rtibbles rtibbles added the bug Behavior is wrong or broken label Mar 22, 2023
@dbnicholson
Copy link
Contributor

It doesn't do completely nothing. It's used in kolibri.utils.main.initialize to provide to Django when loading the settings. Unfortunately, the validation of the --settings CLI option doesn't take it into account, so it effectively doesn't do anything since that's the only reason you'd use --pythonpath.

@rtibbles
Copy link
Member Author

It seems like the appropriate fix here then is to migrate this from kolibri.utils.main.initialize to earlier in the CLI pipeline (before the settings option is processed).

@dbnicholson
Copy link
Contributor

I suppose, but the both the parsed pythonpath and settings option are passed to initialize. It seems like a bit of a circular dependency - you need to parse and process pythonpath before settings. To me the simpler solution is to not do validate_module at option parsing time or maybe not at all since it's only used for the django setup and django will certainly validate it.

@rtibbles
Copy link
Member Author

It's possible with Click to require that one setting is parsed before another, so that would allow pythonpath to be processed first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Behavior is wrong or broken
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants