-
Notifications
You must be signed in to change notification settings - Fork 6
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
EVA-3322 - refactor #48
Conversation
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 this approach makes sense to me, hopefully will encourage us to think more about how we use our internal vs. truly publicly available resources, in addition to the stripped down dependency set for eva-sub-cli...
It's true that it causes some hassle in refactoring/renaming (if we've pinned versions we can at least do it on an as-needed basis?), so if other people think it's not worth the trade-off that would be okay as well.
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.
config_utils needs to be in internal as well
In this proposal I moved all the function that can only be used within EVA to the ebi_eva_internal_pyutils folder.
I also created a list of additional dependencies that are required to use the internal functions
This will make it clear which package require the additional dependencies and which do not.
However it still requires a fair amount of refactoring in the code that uses the internal functions.