-
-
Notifications
You must be signed in to change notification settings - Fork 382
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
Medium-scale refactoring #408
Conversation
Motivation: - smart_open_lib.py was cluttered - parsing logic was messy Main changes: - smart_open.parse_uri is now a publicly available function - Parsers for the individual schemes, e.g. HDFS, S3 now live in the corresponding submodule - Moved compression to a separate submodule I haven't changed any of the unit tests, in order to demonstrate that everything still works as expected from the outside.
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 idea! Having a clear structure will be helpful for external contributors.
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.
Nice work @mpenkov!
@piskvorky @menshikh-iv I've made more changes and made sure the build passes. Can you please re-review? |
|
||
You can confirm the documentation changes by running: | ||
|
||
python -c 'help("smart_open")' |
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.
link to submodule should be here, I guess?
@@ -0,0 +1,132 @@ | |||
# Extending `smart_open` | |||
|
|||
This document targets potential contributors to `smart_open`. |
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.
What's about actions in case if I want to add new format, but without pushing them to smart_open (for example, reader for proprietary stuff in the company, useless for open-source).
Amazing work @mpenkov ⭐ |
Fixes: - piskvorky#391 - piskvorky#409 Related to: - piskvorky#413 - piskvorky#468 - piskvorky#408
Motivation:
Main changes:
corresponding submodule
I haven't changed any of the unit tests, in order to demonstrate that
everything still works as expected from the outside.
Checklist
Before you create the PR, please make sure you have: