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

Fix attr: with package_dirs #1365

Merged
merged 1 commit into from May 16, 2018

Conversation

jmbowman
Copy link
Contributor

@jmbowman jmbowman commented May 15, 2018

Summary of changes

Takes the package_dir option into account when loading version from a module attribute. This should handle cases where all modules are located under a common parent and where that particular module is in a subdirectory and/or has a different name. It doesn't attempt to handle cases where package_dir contains keys like "foo.bar", but I'm not sure such entries are even valid.

This required adding a dependency on the options section parsing in the metadata section parsing, in order to get the value of package_dir. This seems pretty harmless since they happen right after each other, but let me know if it poses any problems.

Closes #1333

Pull Request Checklist

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

@jmbowman jmbowman force-pushed the jmbowman/version_attr_and_package_dir branch from 03270db to 0c2e5c9 Compare May 15, 2018 20:54
if attrs_path[0] in package_dir:
# A custom path was specified for the module we want to import
custom_path = package_dir[attrs_path[0]]
parts = custom_path.rsplit('/', 1)
Copy link
Member

Choose a reason for hiding this comment

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

There probably should be OS directory separator instead of hardcoded /.

Copy link
Contributor Author

@jmbowman jmbowman May 16, 2018

Choose a reason for hiding this comment

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

The setuptools documentation specifies that Unix-style separators should be used for all paths in setup.cfg:

Also notice that if you use paths, you must use a forward slash (/) as the path separator, even if you are on Windows. Setuptools automatically converts slashes to appropriate platform-specific separators at build time.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it is so, and the last sentence is what I was concerned with: I've suspected this automatic normalization is done after the parsing stage, but was not sure.

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'm pretty sure it's after; everything up to this point is mostly config file parsing, without much knowledge of what the individual values represent. Also, AppVeyor builds passed on Windows with the new test cases including forward slashes in package_dir.

@jaraco jaraco merged commit 2d7b28d into pypa:master May 16, 2018
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

3 participants