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

DM-26403: Reorganize ButlerURI into separate files #410

Merged
merged 2 commits into from Oct 30, 2020

Conversation

fubarwrangler
Copy link

Splitting butler URI backends into separate files for ease of eventual splitting off into separate package.

@timj timj changed the title Tickets/dm 26403 DM-26403: Reorganize ButlerURI into separate files Oct 29, 2020
@fubarwrangler fubarwrangler force-pushed the tickets/DM-26403 branch 2 times, most recently from 72669e8 to 6e97d8a Compare October 29, 2020 21:39
@fubarwrangler fubarwrangler marked this pull request as ready for review October 29, 2020 21:48
@@ -33,7 +33,7 @@
Union,
)

from ._butlerUri import ButlerURI
from .uri import ButlerURI
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still wanted the directory to be called _butlerUri -- which would mean this line doesn't change. I understand that the s3utils and webdavutils imports into the tests then look weird but I think I can live with that. In the longer term the s3utils and webdavutils tests should disappear because ButlerURI should be testing all that anyhow. That leaves us only with the test_butler.py imports which need a bit more thought.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You want me to rename the whole uri/ directory to _butlerUri/ then? Do I keep a file called _butlerUri.py in that directory too?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I want the directory renamed and the _butlerUri.py to still be in that subdirectory.

# Should only expose ButlerURI

from ._butlerUri import ButlerURI
__all__ = ("ButlerURI",)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't usually put __all__ in __init__.py since for the way we use __init__.py it mostly is for forwarding symbols anyhow. The __all__ here does not serve any purpose so can be removed.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood, wasn't sure will fix

from .http import ButlerHttpURI
subclass = ButlerHttpURI
elif parsed.scheme == "resource":
# Rules for scheme names disasllow pkg_resource
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor typo in this comment. Maybe that was mine originally 😄

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!

William Strecker-Kellogg added 2 commits October 30, 2020 12:47
Make new _butlerUri directory with files for each class providing an
implementation of a URI-handler
Merge webdavutils.py into http.py for _butlerUri and simply move s3utils
into the folder for now. Someday we can perhaps make s3utils go away but
not today...
@fubarwrangler
Copy link
Author

This should be ready now, rebased into 2 commits

@fubarwrangler fubarwrangler merged commit 2708970 into master Oct 30, 2020
@timj timj deleted the tickets/DM-26403 branch February 16, 2024 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants