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

Replace the export command by the export plugin #4824

Closed
wants to merge 4 commits into from

Conversation

sdispater
Copy link
Member

This PR replaces the existing export command by the poetry-export-plugin.

It also makes changes in the way plugins are handled via the respective plugin commands that is simpler and more reliable. Note that there is currently an issue in the installer/solver that removes installed packages if they are not required by Poetry or any plugin. I am still working on a fix that will be done in a subsequent PR.

Pull Request Check List

  • Added tests for changed code.
  • Updated documentation for changed code.

@sdispater sdispater added area/cli Related to the command line area/plugin-api Related to plugins/plugin API labels Nov 24, 2021
@sdispater sdispater added this to the 1.2 milestone Nov 24, 2021
Copy link
Member

@neersighted neersighted left a comment

Choose a reason for hiding this comment

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

Looks good overall, but it's a very large monolithic change and I think we need to break it up for ease of review, testing, and rollback. Moderate feedback on design patterns, idioms, and code smells. There's some very easy to fix code quality issues that pre-commit should break down easily enough.

My main overall concern is continuing a pattern of delayed/scoped imports. I am generally of the opinion that they are adding significant mental overhead and potential for breakage and are likely to become 'stale' as code is refactored. I would much rather add explicit at-the-start-of-the-file imports and consider a project-wide solution to startup time if it's getting out of control.

Solutions might be PyOxidizer (through the official installer), a Mercurial-style lazy-load-with-exceptions-list, or something else.

Check out these articles for more ideas (and be warned that my knowledge of this field is nascent):
https://gregoryszorc.com/blog/2019/01/10/what-i%27ve-learned-about-optimizing-python/
https://snarky.ca/lazy-importing-in-python-3-7/

@@ -34,7 +34,7 @@ generate-setup-file = false
[tool.poetry.dependencies]
python = "^3.6"

Copy link
Member

Choose a reason for hiding this comment

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

Just curious -- does the export plugin require the current changes in core? If not I'd like to move this change to a new PR as it affects other PRs and changes (e.g. unblocks #4743).

cwd = self.poetry.file.parent
except (PyProjectException, RuntimeError):
cwd = Path.cwd()
cwd = Path.cwd()
Copy link
Member

Choose a reason for hiding this comment

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

This looks unrelated. I'm all for simplification, but could we explain this change/break it out into a separate commit?

src/poetry/console/commands/plugin/add.py Outdated Show resolved Hide resolved
src/poetry/console/commands/plugin/add.py Show resolved Hide resolved
existing_packages = self.get_existing_packages_from_input(
plugins, poetry_content, "dependencies"
existing_plugins = {}
if env_dir.joinpath("plugins.toml").exists():
Copy link
Member

Choose a reason for hiding this comment

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

I do think a separate file looks easier. If I'm reading this right, this is a manifest of known plugins in $POETRY_HOME/the Poetry venv. Does this require a docs change at all? I don't think it does, but I'd like to be sure.

Copy link
Member

Choose a reason for hiding this comment

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

This entire file (and most of these codepaths) are long, imperative, and repeat a fair bit. We're also sprinkling env_dir.joinpath("plugins.toml") everywhere. Have we given any thought to encapsulating all the logic around reading/manipulating this file in a class PluginManifest or similar?

) -> None:
for source in sources:
def create_pool(cls, config: "Config", io: Optional["IO"] = None) -> "Pool":
from poetry.repositories.pool import Pool
Copy link
Member

Choose a reason for hiding this comment

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

Really not sure we should be adding scoped imports at all.

tests/installation/test_installer.py Outdated Show resolved Hide resolved
tests/installation/test_installer.py Outdated Show resolved Hide resolved
@@ -357,7 +357,7 @@ def test_add_directory_constraint(
mocker: "MockerFixture",
):
p = mocker.patch("pathlib.Path.cwd")
p.return_value = Path(__file__).parent
p.return_value = Path(__file__).parent.parent.parent / "fixtures" / "git"
Copy link
Member

Choose a reason for hiding this comment

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

This parent calls are a bit messy, but also I think we should address them as part of a 'fixtures-to-get-fixtures' refactor to the test suite.

Copy link
Member

Choose a reason for hiding this comment

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

note that you can also use Path.parents:

    p.return_value = Path(__file__).parents[2] / "fixtures" / "git"

) -> None:
for source in sources:
def create_pool(cls, config: "Config", io: Optional["IO"] = None) -> "Pool":
from poetry.repositories.pool import Pool
Copy link
Member

Choose a reason for hiding this comment

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

This delayed import really seems useless -- the import will run almost right away anything. I think this pattern tends to be a premature optimization that is hard to maintain and that we should address import overhead with a higher-level/more unified approach if it's an issue.

@neersighted
Copy link
Member

Another question -- did the fixes from #4686 get ported to the exporter plugin? @dimbleby, if not, porting your tests/fixes as well as any insight into the marker-negation regression would be appreciated!

@dimbleby
Copy link
Contributor

#4686 has a testcase, so long as this MR doesn't break that then I guess it's fine.

As for the "marker-negation regression": I believe it's #3660 that causes bar==7.8.9 to be split into bar==7.8.9; platform_system != "Windows" or platform_system == "Windows" in that testcase.

So far as I know #3660 is a good fix, and it just exposes this oddity.

I imagine this could be fixed, if desired, by recognising the special case of taking a union between a SingleMarker and its own inverse, here.

@neersighted
Copy link
Member

#4686 has a testcase, so long as this MR doesn't break that then I guess it's fine.

As for the "marker-negation regression": I believe it's #3660 that causes bar==7.8.9 to be split into bar==7.8.9; platform_system != "Windows" or platform_system == "Windows" in that testcase.

So far as I know #3660 is a good fix, and it just exposes this oddity.

I imagine this could be fixed, if desired, by recognising the special case of taking a union between a SingleMarker and its own inverse, here.

test_exporter.py is completely eliminated in this PR, in favor of a copy that was factored out into the plugin version. I'm still not sure if your fix made the leap (haven't looked yet).

Thanks for the PR -- interesting to know the breakage was in core (and it explains why I had trouble finding it) --- I'll take a look in a bit.

@sdispater sdispater force-pushed the replace-export-by-plugin branch 2 times, most recently from 27c8e4c to 0ed54ab Compare December 19, 2021 19:37
@sdispater sdispater force-pushed the replace-export-by-plugin branch from 0ed54ab to a4485fd Compare December 19, 2021 22:29
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/cli Related to the command line area/plugin-api Related to plugins/plugin API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants