-
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!
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 <[email protected]>
ed19941
to
94c694e
Compare
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
exclude
option, available both via the command line when calling eitherpython -m plux entrypoints
or directly via the low level dist commandpython -c "import setuptools; setuptools.setup()" plugins
command.I've also added
pyproject.toml
parsing in order to not have to pass that via the command line, but instead rely on a new section inpyproject.toml
namedtool.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.toml
section would look like the following (tested in the real world project):TODO
pyproject.toml
section