-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Changes to pyproject for test-pypi #2593
Conversation
include = ["styles/default/*.mpfstyle.json", "styles/default/*.mplrc.json", | ||
"styles/default/*.richstyle.json", "styles/default/*.mplstyle", | ||
"portfolio/*/*.xlsx", "portfolio/*/*.csv", "routines/*.openbb", | ||
"data_sources_default.json","i18n/en.yml"] |
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.
When we bundle it like this the files that we include end up in envs/obb/lib/python3.9/site-packages/
. That's the same place where all other python packages are. What are the odds that we'll have a conflict there, like an existing i18n
or portfolio
folder?
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.
@piiq I would say low if we try to manage our dependencies closely. So hopefully there should be no conflict. Sadly there is no workaround to putting these folders and files into a different location. Additionally, we could potentially change the name of the folders and files that are placed in site-packages, but that would require a major major refactor. Folders like portfolio
and il8n
are necessary to run the terminal via pip install with the command openbb
.
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.
Why do you think there's no workaround?
When i was experimenting with instalability i have moved the files into the openbb_terminal folder. We have core,config folders in there, right?
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.
Sorry I should have phrased better. There doesn't seem to be a workaround if we use our current folder structure. Poetry pollutes the top level site-packages with the contents of "include" and per this poetry support thread it seems to be the default and only supported behavior. Changing the folder structure as you mentioned and move folders like il8n
into openbb_terminal
would allow for a more neat and clean pip install since they will be packaged in the 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.
I'm fine with both solutions
Packing everything inside openbb_terminal seems just a tiny bit tidier
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 for now we'll keep it this way (placing materials into site-packages
), but refactoring the folder structure will be a good idea for a future enhancement. We can track this enhancement with this #2623!
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.
Let's not merge this until we figure out the folders (as agreed in the call)
Description
Updating version, description, readme, license, homepage, repo, documentation for test-pypi. Check it out at: https://test.pypi.org/project/openbbterminal/
Name change from openbbterminal to openbb will most likely be occurring for the actual pypi release.
Others
pre-commit install
.pytest tests/...
.