-
Notifications
You must be signed in to change notification settings - Fork 7
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
Change isbi_utils
from writing files to comparing in memory
#67
Conversation
Code changes looks good. Seems to run a bit slower, which is surprising but in my one live test it did replicate the results of the original 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'm hoping we can make this backwards compatible to prevent having to update any miscellaneous code that may break.
Ideally the functions that used to exist will still exist by name and do what they are supposed to do. Internally those functions can change and call other/new functions to keep them short.
Keep old parameters but just default them to None
so they do nothing. If users try to use them, please warn that the arg is deprecated.
If you could also add a test for |
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.
Apologies for my confusing statements earlier, I got mixed up on the different functions.
I left a bunch of comments, please follow up if anything isn't clear.
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 great! Very pleased with where it is - just one last small change for deprecation warning.
Co-authored-by: willgraf <7930703+willgraf@users.noreply.github.com>
Thanks for all the help! |
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.
LGTM, good changes and well tested.
The only failing checks are for Python 3.5 coverage, which does not seem like a blocker to me.
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 good. Would just like clarification on one thing.
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.
Great work!
What
isbi_utils_tests
to test the new approachWhy