Replace the export command by the export plugin #4824

sdispater · 2021-11-24T22:35:31Z

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.

neersighted

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/

neersighted · 2021-11-25T01:44:53Z

pyproject.toml

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



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).

neersighted · 2021-11-25T01:45:21Z

src/poetry/console/commands/init.py

-            cwd = self.poetry.file.parent
-        except (PyProjectException, RuntimeError):
-            cwd = Path.cwd()
+        cwd = Path.cwd()


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

neersighted · 2021-11-25T01:49:50Z

src/poetry/console/commands/plugin/add.py

-            existing_packages = self.get_existing_packages_from_input(
-                plugins, poetry_content, "dependencies"
+        existing_plugins = {}
+        if env_dir.joinpath("plugins.toml").exists():


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.

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?

neersighted · 2021-11-25T02:20:49Z

src/poetry/factory.py

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


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

tests/installation/test_installer.py

neersighted · 2021-11-25T02:33:01Z

tests/console/commands/test_add.py

@@ -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"


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.

note that you can also use Path.parents:

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

neersighted · 2021-11-25T02:37:42Z

src/poetry/factory.py

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


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 · 2021-11-28T05:52:45Z

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 · 2021-11-28T10:16:15Z

#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 · 2021-11-28T13:27:32Z

#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.

github-actions · 2024-02-29T09:06:37Z

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.

sdispater added area/cli Related to the command line area/plugin-api Related to plugins/plugin API labels Nov 24, 2021

sdispater added this to the 1.2 milestone Nov 24, 2021

neersighted requested changes Nov 25, 2021

View reviewed changes

dimbleby mentioned this pull request Nov 28, 2021

special case union of single marker with inverse python-poetry/poetry-core#255

Merged

2 tasks

sdispater added 2 commits December 19, 2021 20:25

Refactor sources configuration

3bb8c30

Use the export plugin for the export command

7fc06ac

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

Fix linting and styling issues

589c4ad

sdispater force-pushed the replace-export-by-plugin branch from 0ed54ab to a4485fd Compare December 19, 2021 22:29

Refactor access to Poetry home location

7cd37d2

sdispater force-pushed the replace-export-by-plugin branch from a4485fd to 7cd37d2 Compare December 19, 2021 22:39

This was referenced Feb 4, 2022

(🎁) poetry export still has a --dev option which just includes all groups #5140

Closed

Refactor export #5156

Merged

finswimmer mentioned this pull request Mar 31, 2022

Poetry 1.2 export command should acknowledge dependency groups #5367

Closed

abn self-assigned this Mar 31, 2022

abn mentioned this pull request Apr 5, 2022

export: replace in-tree implementation with poetry-plugin-export #5413

Merged

radoering closed this in #5413 Apr 16, 2022

neersighted deleted the replace-export-by-plugin branch August 25, 2022 07:21

github-actions bot locked as resolved and limited conversation to collaborators Feb 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace the export command by the export plugin #4824

Replace the export command by the export plugin #4824

sdispater commented Nov 24, 2021

neersighted left a comment

neersighted Nov 25, 2021

neersighted Nov 25, 2021

neersighted Nov 25, 2021

neersighted Nov 25, 2021

neersighted Nov 25, 2021

neersighted Nov 25, 2021

branchvincent Nov 28, 2021

neersighted Nov 25, 2021

neersighted commented Nov 28, 2021

dimbleby commented Nov 28, 2021

neersighted commented Nov 28, 2021

github-actions bot commented Feb 29, 2024

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

Replace the export command by the export plugin #4824

Replace the export command by the export plugin #4824

Conversation

sdispater commented Nov 24, 2021

Pull Request Check List

neersighted left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neersighted commented Nov 28, 2021

dimbleby commented Nov 28, 2021

neersighted commented Nov 28, 2021

github-actions bot commented Feb 29, 2024