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

Add data_files support in setup.cfg #1520

Merged
merged 3 commits into from Oct 25, 2018
Merged

Add data_files support in setup.cfg #1520

merged 3 commits into from Oct 25, 2018

Conversation

ssato
Copy link
Contributor

@ssato ssato commented Oct 23, 2018

Summary of changes

These commits add data_files support in setup.cfg, discussed in the issue #1189, such like the following.

[options.data_files]
data = data/48x48/logo.png, data/scale/logo.svg
conf =
    site.d/00_default.conf
    host.d/00_default.conf

I think it may close #1189.

Pull Request Checklist

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

)

with get_dist(tmpdir) as dist:
assert dist.data_files == [
Copy link
Member

@pganssle pganssle Oct 23, 2018

Choose a reason for hiding this comment

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

I believe this is failing because dist.data_files gets parsed first to a dict (with arbitrary order on Py < 3.6), then turned into this key/value list thing.

I would look at what the other tests do, but I think a quick fix would be this:

expected = {
    'cfg': ['a/b.conf', 'c/d.conf'],
    'data': ['e/f.dat', 'g/h.dat']
}

with get_dist(tmpdir) as dist:
    # Order independent comparison
    assert dict(dist.data_files) == expected

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks a lot for your suggestion! I completely forgot about that. I'll commit the fix into this my branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I committed the fix based on your suggestion but data type of expected result is kept.

In the test case, dist.data_files needs to be sorted because the
current implementation loads the configuration files as a dictionary
with arbitrary order on Python < 3.6.
This adds the `[options.data_files]` section to the existing setup.cfg
example.
Copy link
Member

@pganssle pganssle left a comment

Choose a reason for hiding this comment

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

I've rebased this branch against master and squashed together the fixup commits, plus touched up some of the commit messages and the changelog entry, so everything looks good to me.

Thanks @ssato for your PR!

@benoit-pierre
Copy link
Member

The documentation should be updated.

@pganssle
Copy link
Member

@benoit-pierre Good catch, I guess we need to add it to this table. Anywhere else?

@benoit-pierre
Copy link
Member

Yes, and I would add a new column to the table mention the minimum supported version.

@pganssle
Copy link
Member

@ssato If you want to update the documentation, please do a force-pull from your branch, since I've rewritten the PR's history since your last commit, and that will cause conflicts.

@ssato
Copy link
Contributor Author

ssato commented Oct 24, 2018

@ssato If you want to update the documentation, please do a force-pull from your branch, since I've rewritten the PR's history since your last commit, and that will cause conflicts.

@pganssle Thanks a lot for the cleanups! It looks much better ;-)
I'll not update my branch and keep as it is now, just wait for the merge.

Copy link
Member

@jaraco jaraco left a comment

Choose a reason for hiding this comment

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

I think there's still an outstanding request to update the documentation. @ssato would you do that please?

@pganssle
Copy link
Member

@jaraco @benoit-pierre Normally I'd block on getting an update, but given that we're doing a sprint this weekend, I'm tempted to just merge this as-is and leave the documentation update as an easy [good first issue].

If no one takes it at the sprints this weekend, I can do it myself on Monday, but frankly it's nice to have some very simple fixes that you can use as a demo PR for new contributors.

@jaraco
Copy link
Member

jaraco commented Oct 25, 2018

Works for me. @pganssle Would you file that ticket?

@pganssle
Copy link
Member

Filed #1522.

@dirk-thomas
Copy link

Since distutils on the very lowest level replaces - in keys with _ (https://github.com/python/cpython/blob/31ec52a9afedd77e36a3ddc31c4c45664b8ac410/Lib/distutils/dist.py#L414) this can't be used to install data files into a location which contains dashes.

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.

data_files support in setup.cfg
5 participants