Skip to content

Add data_files support in setup.cfg#1520

Merged
jaraco merged 3 commits into
pypa:masterfrom
ssato:data_files_2
Oct 25, 2018
Merged

Add data_files support in setup.cfg#1520
jaraco merged 3 commits into
pypa:masterfrom
ssato:data_files_2

Conversation

@ssato

@ssato ssato commented Oct 23, 2018

Copy link
Copy Markdown
Contributor

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

Comment thread setuptools/tests/test_config.py Outdated
)

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

@pganssle pganssle Oct 23, 2018

Copy link
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
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.

ssato added 3 commits October 24, 2018 12:10
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.

@pganssle pganssle left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
Member

The documentation should be updated.

@pganssle

Copy link
Copy Markdown
Member

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

@benoit-pierre

Copy link
Copy Markdown
Member

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

@pganssle

Copy link
Copy Markdown
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

ssato commented Oct 24, 2018

Copy link
Copy Markdown
Contributor Author

@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.

@jaraco jaraco left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
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

jaraco commented Oct 25, 2018

Copy link
Copy Markdown
Member

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

@pganssle

Copy link
Copy Markdown
Member

Filed #1522.

@dirk-thomas

Copy link
Copy Markdown

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