-
Notifications
You must be signed in to change notification settings - Fork 0
Add Exclude option when building entrypoints #28
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
Conversation
|
|
||
| def run(self) -> None: | ||
| plugin_finder = PluginFromPackageFinder(DistributionPackageFinder(self.distribution)) | ||
| plugin_finder = PluginFromPackageFinder(DistributionPackageFinder(self.distribution, exclude=self.exclude)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm actually not too sure if we should filter at the Package level or the Module level. Module level might be a bit more flexible, right now we have to ignore the full package and specifying a module name won't work.
It is currently more in line with setuptools.find_packages and the other PluginFinder, but doesn't exactly serve the same purpose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't thought very long about, but I think either can work. This looks good!
thrau
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice PR @bentsku, thank you! The exclude flags have not been used consistently at all, and haven't been handed through to the various layers properly, so I'm really happy to see we have it implemented through CLI, to config, to plugin finding! Having excludes work properly is essentially for having an error-free build process. 💯
I only have a couple of minor comments around cosmetics and docs, so nothing blocking.
|
|
||
| def run(self) -> None: | ||
| plugin_finder = PluginFromPackageFinder(DistributionPackageFinder(self.distribution)) | ||
| plugin_finder = PluginFromPackageFinder(DistributionPackageFinder(self.distribution, exclude=self.exclude)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't thought very long about, but I think either can work. This looks good!
| def __init__(self, distribution: Distribution): | ||
| def __init__(self, distribution: Distribution, exclude: t.Optional[t.Iterable[str]] = None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we update the pydoc of _PackageFinder, to add a couple of sentences + example how the exclude works?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do! However, beware, I've only updated the DistributionPackageFinder, as the regular _PackageFinder and its subclass DefaultPackageFinder does already support exclude via setuptools.find_namespace_packages and setuptools.find_packages (however the CLI does not pass the where/exclude/include options, it has some default values that I did not want to change)
I've found that we are using the DistributionPackageFinder everywhere when building entrypoints, and are only using the DefaultPackageFinder with the PackagePathPluginFinder that is only used in the discover CLI command. I'm not entirely sure why it is not unified, and I did not find documentation of the discover CLI command, so I haven't touched this part of the codebase. It also doesn't seem as important.
I can open a follow-up PR to improve the discover command but not it is worth it at the moment?
Co-authored-by: Thomas Rausch <thomas@thrau.at>
ed19941 to
94c694e
Compare
bentsku
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the review, I've addressed all comments 🙏 there might be a follow up for PackagePathPluginFinder but I think it's somewhat a bit out of scope of this PR, as it is only used in the discover CLI command. If you want me to fix it too, I can do so in a follow up 😄
@thrau will you do a release once merged?
| def __init__(self, distribution: Distribution): | ||
| def __init__(self, distribution: Distribution, exclude: t.Optional[t.Iterable[str]] = None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do! However, beware, I've only updated the DistributionPackageFinder, as the regular _PackageFinder and its subclass DefaultPackageFinder does already support exclude via setuptools.find_namespace_packages and setuptools.find_packages (however the CLI does not pass the where/exclude/include options, it has some default values that I did not want to change)
I've found that we are using the DistributionPackageFinder everywhere when building entrypoints, and are only using the DefaultPackageFinder with the PackagePathPluginFinder that is only used in the discover CLI command. I'm not entirely sure why it is not unified, and I did not find documentation of the discover CLI command, so I haven't touched this part of the codebase. It also doesn't seem as important.
I can open a follow-up PR to improve the discover command but not it is worth it at the moment?
| :param exclude: the shell style wildcard patterns to exclude | ||
| :param include: the shell style wildcard patterns to include |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as I copied the code for setuptools to find package, I was a bit surprised to see glob here. I checked and the documentation of setuptools mentions the following:
'exclude' is a sequence of names to exclude; '*' can be used
as a wildcard in the names.
When finding packages, 'foo.*' will exclude all subpackages of 'foo'
(but not 'foo' itself).
'include' is a sequence of names to include.
If it's specified, only the named items will be included.
If it's not specified, all found items will be included.
'include' can contain shell style wildcard patterns just like
'exclude'.
So I've used the same wording
While building entry points in a project where we needed to use Alembic, it came to light that the library needs to have a script that cannot be imported and will always fail if imported by anything else than the Alembic library itself with its custom Python loading mechanism.
Plux already had some mechanism in places to ignore some paths, but those were relying on what the end package data would be. Those script files should still be part of the distribution, but not be scanned for plugins.
I've tried to add an
excludeoption, available both via the command line when calling eitherpython -m plux entrypointsor directly via the low level dist commandpython -c "import setuptools; setuptools.setup()" pluginscommand.I've also added
pyproject.tomlparsing in order to not have to pass that via the command line, but instead rely on a new section inpyproject.tomlnamedtool.plux. I am certain we will and probably should extend it further then.I've also verified that this would fix the issue in our project.
The
pyproject.tomlsection would look like the following (tested in the real world project):TODO
pyproject.tomlsection