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

Recursively get package data #1496

Closed
wants to merge 1 commit into from

Conversation

joeflack4
Copy link
Contributor

Summary of changes

Recursively get package data

This should be called from within setup.py's setuptools.setup() function, e.g. package_data={'my_package': get_pkg_data(pkg_name='my_pckage', ...)}

Pull Request Checklist

  • 1. Changes have tests
  • 2. News fragment added in changelog. See documentation for details

Additional details

Will be getting to (1) and (2) shortly.

    This should be called from within setup.py's setuptools.setup() function,
    e.g. package_data={'my_package': get_pkg_data(pkg_name='my_pckage', ...)}
@pganssle
Copy link
Member

Hmmm. I do think it's reasonable to have a package data equivalent of find_packages, but should it maybe be called find_pkg_data?

That said, this is a new public API to maintain, and I find find_packages somewhat dangerous as it is (case in point: flask-admin/flask-admin#1706). I do worry that we're going to be in a similarly dangerous situation here (though TBH accidentally installing extra package data is probably better than accidentally installing extra packages).

One thing to keep in mind is that we should probably try and keep this consistent with importlib.resources, since that seems to be the "new way" to get data resources.

In any case, we may want to spend a bit of time to discuss this before you spend too much more time on it @joeflack4.

@jaraco @benoit-pierre @warsaw Any thoughts on this?

@joeflack4
Copy link
Contributor Author

Thanks for the consideration. Definitely you guys can let me know if you want me to work on this implementation in any of the ways so far mentioned (find_pkg_data and possibly importlib.resources sound like good ideas to me), or other ways.

I don't have much experience on the folly of expanding the public API. I feel though like this would be a first step in lowering the barrier of entry into packaging, which would make for more packages on PyPi (or internal builds), which I think would be overall a good thing. I think a nicer implementation would be to just accept glob patterns into package_data directly, but I know that would be a much tougher sell (and harder to implement).

@pganssle pganssle added the Needs Triage Issues that need to be evaluated for severity and status. label Oct 19, 2018
@jaraco
Copy link
Member

jaraco commented Dec 14, 2018

Can you explain more about your use-case that motivated this change? Is there a reason that include_package_data=True isn't sufficient?

I'm also concerned about this change because it introduces a pattern for more imperative config in setup.py, expanding the difference between what's possible with setup.py and what's possible with setup.cfg (declarative config). Ideally, those two interfaces would support equivalent functionality.

@joeflack4
Copy link
Contributor Author

That's a good point, on setup.cfg and setup.py. I would agree that if they're feature set is in sync now, it should be kept that way.

The use case for recursively adding package data is to make including package data easier / more maintainable. If I have a single folder that has a deeply nested tree structure, then without using recursion, I have to go through and manually list out each "leaf" (bottom level folder in the hierarchy) individually. And, if the folder structure changes, then this now breaks my import of package_data in setuptools. I have to remember to go back and edit the list to adhere to my new structure. In my case, I we forgot to do that, and we deployed without package_data included, causing bugs.

As far as implementation goes, I think ideally, a glob pattern would be better than a recursive function, but I would entrust that implementation to someone more familiar with setuptools.

@jaraco
Copy link
Member

jaraco commented Oct 27, 2019

@joeflack4 Can you provide an example package that illustrates the use-case (that's currently clumsy and would be more streamlined with this change)?

@joeflack4
Copy link
Contributor Author

@jaraco I'd love to!

I brought this up because I have a package that serves as a Python API and CLI, and another package that is a web app. As background, the software converts certain kinds of Excel files into Word documents, etc. There are templates that allow for different documents format, and those are the package data I'm trying to include. Each template has its own folder, and tends to have subfolders.

@jaraco
Copy link
Member

jaraco commented Oct 28, 2019

I think this PR relates to #1806.

@joeflack4
Copy link
Contributor Author

Oh yes, definitely.

Wow, I didn't even know that glob() supported recursion. Yeah, if you can just support it that way, that would be more ideal than my PR here.

@jaraco
Copy link
Member

jaraco commented Dec 31, 2019

It sounds like a better approach here could be to add support for recursive globs. Would you explore that approach?

@jaraco jaraco closed this Dec 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Triage Issues that need to be evaluated for severity and status.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants