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 findns: directive to config #1420

Closed
wants to merge 10 commits into from

Conversation

silkentrance
Copy link
Contributor

@silkentrance silkentrance commented Jul 11, 2018

Summary of changes

  • extend documentation on new findns: directive
  • realize new findns: directive

Closes #1419

Pull Request Checklist

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

@silkentrance
Copy link
Contributor Author

@pganssle Have a look and tell me if that is the right direction. I so, I will continue with implementing required tests.

@pganssle
Copy link
Member

Is there a reason it can't be find_namespace? I'm even dubious about the find_packages_ns name, since it seems too obscure to me, I'd rather it be find_namespace_packages or something.

@silkentrance
Copy link
Contributor Author

@pganssle can we handle this in a different issue/PR?

@silkentrance
Copy link
Contributor Author

As for the directive, sure, this I can rename right now.

@silkentrance silkentrance force-pushed the gh-1419 branch 2 times, most recently from 42c3da2 to 81ae059 Compare July 11, 2018 17:34
@pganssle
Copy link
Member

The find_packages_ns stuff is indeed a completely different discussion so don't worry about that.

@pganssle
Copy link
Member

This seems like it will work, but the implementation might be a bit cleaner if you do this:

directive = value.split(':', 1)[0]
if directive not in {'find', 'find_namespace'}:
    ...

@silkentrance
Copy link
Contributor Author

silkentrance commented Jul 11, 2018

@pganssle good point, but this will be a different commit
but still I have to find out whether it is find_namespace... will think about that

@silkentrance
Copy link
Contributor Author

silkentrance commented Jul 11, 2018

@pganssle note that with the latest commit I refrained from using string.split() and instead used value.strip(). and maybe even this op is redundant as the parser might have normalized the value already, but we can never know...

Copy link
Contributor

@agronholm agronholm left a comment

Choose a reason for hiding this comment

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

Minor things

@@ -2389,8 +2389,8 @@ Metadata and options are set in the config sections of the same name.
* In some cases, complex values can be provided in dedicated subsections for
clarity.

* Some keys allow ``file:``, ``attr:``, and ``find:`` directives in order to
cover common usecases.
* Some keys allow ``file:``, ``attr:``, and ``find:`` and ``find_namespace:`` directives i
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo at the end?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will look into this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

if not value.startswith(find_directive):
return self._parse_list(value)
if not trimmed_value in find_directives:
return self._parse_list(value)
Copy link
Contributor

Choose a reason for hiding this comment

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

Bad indentation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops nice find

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and there was even more where this came from...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


return find_packages(**find_kwargs)

def parse_section_packages__find(self, section_options):
"""Parses `packages.find` configuration file section.
"""Parses `packages.find[ns]` configuration file section.
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this really use a packages.findns section? The tests do not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice find, fixing right now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

' fake_package.sub_one\n'
)
with get_dist(tmpdir) as dist:
assert set(dist.packages) == set(
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use a set literal here. Python 2.6 support was dropped in v37.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The other tests also use the non set literal, should I proceed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@silkentrance
Copy link
Contributor Author

If you comply to the changes, I will proceed with fixing #1421 based upon these changes.


py2_only = pytest.mark.xfail(not PY2, reason="Test runs on Python 2 only")
Copy link
Member

Choose a reason for hiding this comment

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

I think these should be pytest.mark.skip, not xfail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will have to fix the tests for find_packages_ns as well. you would want to fix the other tests that use that pattern.

Copy link
Contributor

Choose a reason for hiding this comment

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

I concur.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

six.PY2 seems to fail when marking tests for specific versions

SKIP [1] setuptools/tests/test_config.py:604: Test runs on Python 2 only

Copy link
Member

Choose a reason for hiding this comment

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

I think that's indicating that the test was skipped. When you mark something xfail, it actually runs the test and ignores the result if it fails (or if you make it a strict xfail, actually fails the test suite if the test passes). skip doesn't run the test at all, so I believe that message is expected.

You are right that we should be switching many xfails over to skip. I was planning on doing that at some point, but it takes time to figure out which ones are intended as an actual xfail (which generally means "this should work but it's a known bug - when this test starts passing drop this decorator") and which are just using the wrong decorator for skip.

Copy link
Contributor

@agronholm agronholm left a comment

Choose a reason for hiding this comment

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

Looks alright now!

@silkentrance
Copy link
Contributor Author

silkentrance commented Jul 11, 2018

currently working on py2_only/py3_only for test_config. It does not seem to work as expected, reporting false results.

tomorrow will be a new day...

@pganssle
Copy link
Member

@silkentrance That's my mistake, the correct mark is skipif, skip skips unconditionally (though I'm not sure why it accepts a random argument).

@pganssle
Copy link
Member

I pushed an updated version with the proper decorator to your branch.

@silkentrance
Copy link
Contributor Author

@pganssle thanks. I did find out about skipif in the morning, too. Yesterday I was way too tired...

@silkentrance
Copy link
Contributor Author

silkentrance commented Jul 12, 2018

I have refactored out the markers for py2/py3 into tests/init.py since they had been redeclared. Other test modules can be refactored, too, but I leave it at that for the time being.

silkentrance added a commit to coldrye-collaboration/setuptools that referenced this pull request Jul 12, 2018
silkentrance added a commit to coldrye-collaboration/setuptools that referenced this pull request Jul 14, 2018
silkentrance added a commit to coldrye-collaboration/setuptools that referenced this pull request Jul 18, 2018
pganssle pushed a commit to coldrye-collaboration/setuptools that referenced this pull request Aug 11, 2018
pganssle pushed a commit to coldrye-collaboration/setuptools that referenced this pull request Aug 11, 2018
@silkentrance silkentrance deleted the gh-1419 branch August 14, 2018 20:30
silkentrance added a commit to coldrye-collaboration/setuptools that referenced this pull request Aug 14, 2018
silkentrance added a commit to coldrye-collaboration/setuptools that referenced this pull request Aug 14, 2018
pganssle pushed a commit that referenced this pull request Aug 17, 2018
* fix #1419 PEP420: add find_namespace: directive

* fix #1419 PEP420: add find_namespace: directive to documentation

* fix #1419 PEP420: add tests

* fix #1419 PEP420: clean up code

* fix #1419 PEP420: fix typo in documentation

* fix #1419 PEP420: fix typo in documentation

* fix #1419 PEP420: clean up code

* fix #1419 PEP420: add changelog entry

* fixup! fix #1419 PEP420: add tests

* fix #1419 PEP420: cleanup code refactor markers

* #1420: Rename find_namespace_ns to find_namespace_packages

*  #1420: update changelog entry
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.

3 participants