{"action":"created","issue":{"url":"https://api.github.com/repos/brettcannon/importlib_resources/issues/26","repository_url":"https://api.github.com/repos/brettcannon/importlib_resources","labels_url":"https://api.github.com/repos/brettcannon/importlib_resources/issues/26/labels{/name}","comments_url":"https://api.github.com/repos/brettcannon/importlib_resources/issues/26/comments","events_url":"https://api.github.com/repos/brettcannon/importlib_resources/issues/26/events","html_url":"https://github.com/brettcannon/importlib_resources/issues/26","id":273512702,"number":26,"title":"Thoughts on the current API","user":{"login":"warsaw","id":210184,"avatar_url":"https://avatars0.githubusercontent.com/u/210184?v=4","gravatar_id":"","url":"https://api.github.com/users/warsaw","html_url":"https://github.com/warsaw","followers_url":"https://api.github.com/users/warsaw/followers","following_url":"https://api.github.com/users/warsaw/following{/other_user}","gists_url":"https://api.github.com/users/warsaw/gists{/gist_id}","starred_url":"https://api.github.com/users/warsaw/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/warsaw/subscriptions","organizations_url":"https://api.github.com/users/warsaw/orgs","repos_url":"https://api.github.com/users/warsaw/repos","events_url":"https://api.github.com/users/warsaw/events{/privacy}","received_events_url":"https://api.github.com/users/warsaw/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":14,"created_at":"2017-11-13T17:27:27Z","updated_at":"2017-11-16T19:47:47Z","closed_at":null,"author_association":"COLLABORATOR","body":"Over the weekend, I wrote an [experimental branch](https://gitlab.com/mailman/mailman/merge_requests/345/diffs) to switch Mailman 3 to importlib_resources.  Sorry for the catch-all issue, but I want to at least capture my thoughts on the current API.  We can turn these into separate issues if/when we want to change anything.  And at least this should serve as a good conversion guide for #16 .\r\n\r\n* `open()` isn't a very good name, especially if you tend to use `from importlib_resources import` style imports.  I do like the shorter name, but the problem is obvious: this shadows the built-in `open()` function.  In my branch, I've used `from importlib_resources import open as resources_open` but that's less than ideal.  As `open()` is also the obvious choice for the function name, I don't yet have a good suggested alternative.\r\n\r\n* `path()` returning a `pathlib.Path` is problematic.  It's likely the right thing to do, but it means that conversion requires the liberal use of `str()` wrapping, since much existing code is expecting a concrete `str` object.  Maybe this is better in 3.7 (I only tested with 3.6) for stdlib APIs, but won't help you much for internal project APIs that expect a string.   Interestingly, this tends to be a bit easier if you use an `ExitStack` rather than a `with`-statement.  Compare:\r\n\r\n```python\r\n        with path('mailman.testing', 'config-with-instances.pck') as pckpath:\r\n            pckfile = str(pckpath)\r\n```\r\n\r\nand\r\n\r\n```python\r\n        with ExitStack() as resources:\r\n            pckfile = str(resources.enter_context(\r\n                path('mailman.testing', 'config.pck')))\r\n```\r\n\r\n* Returning a context from `path()` is often inconvenient.   Of course, I understand why we have to do that, but ensuring that the context is properly cleaned up can be tricky, and it's certainly much less convenient at times.  I've had to change a few internal APIs to accept an `ExitStack` argument, and then add the `path` context to that, since the resources have to be managed several call frames higher.  E.g.\r\n\r\n```python\r\n@public\r\ndef use_test_organizational_data(resources):\r\n    # Point the organizational URL to our test data.\r\n    filename = str(resources.enter_context(\r\n        path('mailman.rules.tests.data', 'org_domain.txt')))\r\n    url = 'file:///{}'.format(filename)\r\n    return resources.enter_context(\r\n        configuration('dmarc', org_domain_data_url=url))\r\n```\r\n\r\n* Missing API.  I couldn't totally rid myself of `pkg_resources` because there's no current alternative to `resource_isdir()` and `resource_listdir()`.  \r\n\r\n* `read()` should support binary reads.  I know we have `open()` for that, but it turns out to be convenient if `read(encoding=None)` returned bytes.  I know we talked about that before, but it's become more obvious when using this in real-world code.   Conversely, it's still a bit inconvenient that `open()` only returns a file opened in binary mode.  Fortunately, there's `email.message_from_binary_file()` where I was previously using `email.message_from_file()`, so it wasn't difficult to fix, but it does mean that converting from `pkg_resources` to `importlib_resources` is less mechanical."},"comment":{"url":"https://api.github.com/repos/brettcannon/importlib_resources/issues/comments/345041649","html_url":"https://github.com/brettcannon/importlib_resources/issues/26#issuecomment-345041649","issue_url":"https://api.github.com/repos/brettcannon/importlib_resources/issues/26","id":345041649,"user":{"login":"brettcannon","id":54418,"avatar_url":"https://avatars0.githubusercontent.com/u/54418?v=4","gravatar_id":"","url":"https://api.github.com/users/brettcannon","html_url":"https://github.com/brettcannon","followers_url":"https://api.github.com/users/brettcannon/followers","following_url":"https://api.github.com/users/brettcannon/following{/other_user}","gists_url":"https://api.github.com/users/brettcannon/gists{/gist_id}","starred_url":"https://api.github.com/users/brettcannon/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/brettcannon/subscriptions","organizations_url":"https://api.github.com/users/brettcannon/orgs","repos_url":"https://api.github.com/users/brettcannon/repos","events_url":"https://api.github.com/users/brettcannon/events{/privacy}","received_events_url":"https://api.github.com/users/brettcannon/received_events","type":"User","site_admin":false},"created_at":"2017-11-16T19:47:47Z","updated_at":"2017-11-16T19:47:47Z","author_association":"OWNER","body":"RE: _\"contents() returns an Iterable\"_: yep, that was a typo on my part. 😄 \r\n\r\nRE: _\"although I'd like a little more detail on why you think this can't be Iterable[Path]\"_\": Directories will not work with `open()` or `path()` (unless you're now saying you don't want to return directories, in which case `is_dir()` isn't necessary since you simply won't know they exist through this API). Plus I personally view `pathlib.Path` objects as representing something where actual manipulation of the path -- e.g. getting the parent -- still leads to a valid path, which is not guaranteed in this instance if you're working with a zip file.\r\n\r\nRE: _\"I still like read() and our current implementation of it is more than one line of code\"_: True, it's more than one line at 5 statements in total. But if we add the string-opening feature that you want for `open()` (which I covered in my proposed API change), the code would become:\r\n```python\r\ndef read(package, file_name, encoding='utf-8', errors='strict'):\r\n    with open_resource(package, file_name, encoding=encoding, errors=errors) as file:\r\n        return file.read()\r\n```\r\nSo while I will concede 2 > 1, you're also leaving out the tests and documentation in that line count that we will have to maintain. 😉 \r\n\r\nRE: _\"\"contents\" feels a bit ambiguous\"_: Sure, but `children()` is also doesn't feel quite right as you're not returning the children of a package, but what resources a package contains. I actually almost named it `resources()`, but I figured `importlib_resources.resources()` seemed a bit redundant. If you don't think so then let's just go with that name.\r\n\r\nAnd your proposed change for `is_file()` won't work as now you're breaking the API contract that the package controls what resources it contains. Your proposal would suddenly have `children()` accept arbitrary `Path` objects and having to know how to work with them. And conflating packages/modules and files is part of what has led to import having this weird situation where so much is treated as if they are files but are in fact not when they are e.g. contained in a zip file. This was actually one of the reasons we chose to only accept file names instead of relative paths in the beginning. IOW I really do not want to have this API suddenly let people arbitrarily work around packages if they are going to be the anchor point for paths because that's when you get people doing stuff that isn't portable across loaders/storage back-ends."}}
{"actor":{"display_login":"brettcannon"}}