Initial implementation of Poetry plugin #32

ThatXliner · 2021-07-03T22:10:24Z

Closes #14

ThatXliner · 2021-07-03T23:47:24Z

Should we notify if a command tries to/could've override Poetry default commands? like

diff --git a/poethepoet/plugin.py b/poethepoet/plugin.py
index b6228d5..86a26eb 100644
--- a/poethepoet/plugin.py
+++ b/poethepoet/plugin.py
@@ -12,13 +12,9 @@ from . import config
 
 
 def get_poe_task_names() -> Iterator[str]:
-    return (
-        name
-        for name in tomlkit.loads(
-            Path(config.PoeConfig().find_pyproject_toml()).read_text()
-        )["tool"]["poe"]["tasks"].keys()
-        if name not in app.COMMANDS
-    )
+    yield from tomlkit.loads(
+        Path(config.PoeConfig().find_pyproject_toml()).read_text()
+    )["tool"]["poe"]["tasks"].keys()
 
 
 def run_poe_task(name: str) -> int:
@@ -27,7 +23,13 @@ def run_poe_task(name: str) -> int:
 
 class PoePlugin(ApplicationPlugin):
     def activate(self, application: app.Application) -> None:
+        io = application.create_io()
         for task_name in get_poe_task_names():  # Assume this function exists somewhere
+            if task_name in app.COMMANDS:
+                io.write_line(
+                    f"<info>Skipped command creation for task '{task_name}'</info>"
+                )
+                continue
             application.command_loader.register_factory(
                 task_name,
                 type(

nat-n · 2021-07-04T21:08:38Z

Interesting. Thanks for making this PoC. I thought it would be more complex to get something working (probably the plugin API I looked at is no longer current).

I'll need to do quite a bit of testing before merging/releasing this (and I'm afraid I have other stuff to test first; currently I don't have a lot of time for this :( ), but I'm glad to see it's not too complicated to get working.

I'm of two minds about whether it's better to namespace poe tasks (i.e. poetry poe test vs poetry test). Though maybe offering a plugin for each and letting the user decide would be reasonable. At least notifying the user if they've configured colliding task name would be in keeping with current design philosophy.

I'd prefer to keep poetry as an extra dependency (i.e. installed via pip install poethepoet[poetry_plugin] or so), because I've found poe is actually useful in contexts without poetry.

The functionality you've demonstrated here might be enough for a first release that adds value, but ultimately some thought needs to go into how to best integrate poe CLI functionality with the poetry CLI, for consistently displaying help/errors etc. This is were I imagined most of the work/refactoring will be required, because poe doesn't currently use cleo.

ThatXliner · 2021-07-04T22:53:28Z

I'm of two minds about whether it's better to namespace poe tasks (i.e. poetry poe test vs poetry test). Though maybe offering a plugin for each and letting the user decide would be reasonable. At least notifying the user if they've configured colliding task name would be in keeping with current design philosophy.

I realized that we probably can move this plugin functionality to a different PyPi package. I'm pretty sure when someone does pip install poethepoet, poethepoet is available as a library (due to Poetry's auto detection I think).

I'd prefer to keep poetry as an extra dependency (i.e. installed via pip install poethepoet[poetry_plugin] or so), because I've found poe is actually useful in contexts without poetry.

Check out my latest commit. I did more than just edit the pyproject.toml file.

The functionality you've demonstrated here might be enough for a first release that adds value, but ultimately some thought needs to go into how to best integrate poe CLI functionality with the poetry CLI, for consistently displaying help/errors etc. This is were I imagined most of the work/refactoring will be required, because poe doesn't currently use cleo.

I see. Can't we just redirect subprocess's stdout and stderr some how?

ThatXliner · 2021-07-04T23:15:03Z

I see. Can't we just redirect subprocess's stdout and stderr some how?

I did that in the latest commit (not so sure if I should use copy.copy or that the variable already copies the value)

ThatXliner · 2021-08-21T17:26:50Z

@nat-n can you review this? Also, I had a thought about separating plug-in (then I wouldn’t need to rely on some big hacks to make poetry an optional dependency)

nat-n · 2021-08-21T20:46:38Z

Hi @ThatXliner, Sorry for the delay. I will come back to this, it's an important feature. I'm afraid I don't expect to have time to give it enough focused attention for at least a couple more weeks. I'm afraid another project has been taking all my time for side projects.

ThatXliner · 2021-09-24T21:07:51Z

I guess I could make a plugin separately, as I said before…

nat-n · 2021-09-26T19:49:51Z

Hi @ThatXliner, sorry again for the delay. If you're dying to ship this then indeed it might be best to build it as a separate project.

I absolutely want to have poetry plugin support built into poethepoet. However by current my assessment it's not a quick win; I need to take some time to get a deep understanding of the plugin architecture and conventions in poetry, and think about how to integrate it properly. Meanwhile my highest priority for this project at the moment is to stabilise the major new features that are currently in pre-release on the development branch so that they can be released as 0.11.0, and this might still take a while.

That said it might be useful, when I'm ready to work on this, to have an example to base it on that's already been battle tested.

nat-n · 2021-09-26T19:52:31Z

Also if you maintain this PR then please target the development branch, as per the contributing guidelines

pyproject.toml

nat-n

I've finally gotten around to looking into this. Sorry for the delay. I anticipate having some time over the next few weeks to get it done (it's the highest priority major feature now after #45).

To start with I'd like your help to understand what you've done so far, to see what's there and what's missing.

nat-n · 2021-12-21T18:53:15Z

poethepoet/plugin.py

+    io.error_output.fileno = sys.stdout.fileno
+    stderr = io.error_output
+
+    return subprocess.run(


A subprocess incurs a performance penalty, and adds some uncertainty due to going via the PATH, complexity around passing args to tasks, etc. Any specific reason not to import and invoke the PoeThePoet class directly?

We can do that? I've never knew.

My thought was that poetry X was just a simple shorthand for poe X. To run the class, there's the IO stuff if we wanted tighter integration

I see. Maybe the optimal solution would be attempt to use that undocumented API, and fallback to loading the config again upon failure.

nat-n · 2021-12-21T18:54:45Z

poethepoet/plugin.py

+
+
+def run_poe_task(name: str, io) -> int:
+    io.fileno = sys.stdin.fileno  # Monkey patches io because subprocess needs it


Why do we need to redirect the io here instead of just letting the subprocess inherit sys.stdout/in/err etc?

Well, I said something here

The functionality you've demonstrated here might be enough for a first release that adds value, but ultimately some thought needs to go into how to best integrate poe CLI functionality with the poetry CLI, for consistently displaying help/errors etc. This is were I imagined most of the work/refactoring will be required, because poe doesn't currently use cleo.

I see. Can't we just redirect subprocess's stdout and stderr some how?

nat-n · 2021-12-21T18:59:59Z

poethepoet/plugin.py

+            sys.exit(1)
+        io = application.create_io()
+        # Create seperate commands per task
+        for task_name in get_poe_task_names():


This is clever, but I wonder what happens if a task name happens to conflict with a native poetry command? Does it fail? does it silently override the poetry command for this project? Have you tried it?

It might be that this is a nice to have, but shouldn't be the default behaviour.

"Does it fail?" yes. I have tried it.

It also says here:

A plugin must not remove or modify in any way the core commands of Poetry.

nat-n · 2021-12-21T19:01:04Z

poethepoet/plugin.py

+    def activate(self, application: app.Application) -> None:
+        if app.Application is None:
+            print(
+                "Poetry not found. Did you install this plugin via `poetry plugin add poethepoet[poetry_plugin]`?"


When can this happen? Is there a known scenario that something other than poetry would attempt to activate this plugin.

#32 (comment)

nat-n · 2021-12-21T19:22:42Z

poethepoet/plugin.py

+    return (
+        name
+        for name in tomlkit.loads(
+            Path(config.PoeConfig().find_pyproject_toml()).read_text()


I haven't tried it but looking through the code it looks like there's a way to get the pyproject.toml content loaded by poetry without loading it again. Something like app.config.config

Interesting. But I wouldn't use anything undocumented. Made a discussion here

nat-n · 2021-12-21T19:26:49Z

poethepoet/plugin.py

+    # please mypy,
+    # and please Poetry's plugin system (simulate the ApplicationPlugin object)
+    app = type("application", (), {"Application": None})  # pylint: disable=C0103
+    ApplicationPlugin = type(


It looks like this try/except is just about making this module importable other than by poetry. Do you have a use case in mind for this?

Good point. This module shouldn't be imported unless as a poetry plugin. Unless there is some wanted flexibility in the future? (But I guess we can always add it back)

Co-authored-by: nat <n@natn.me>

nat-n · 2021-12-23T14:22:49Z

I had a quick play around with this implementation and made a few changes in a new PR. #46

If you'd like to continue to experiment and share ideas on this topic then we can keep this PR open, but id's suggest rebasing it onto the other way and updating it to target the branch of the other one. Otherwise we should close this one now. Or it might be easier to create a new PR of that nature since the rebase (which includes everything from the development branch) is non-trivial. Up to you @ThatXliner

Thanks again for your work on this.

…at-n#30) For example setting: [tool.poe.env] PORT.default = "9000" Will set the PORT variable on the environment, only if it isn't already set. Also replace dependency on more lightweight a complete toml parser: tomli

ThatXliner · 2021-12-24T05:10:38Z

Rebased... I think

…ence (nat-n#40) Allow two addition option value for `ingore_fail` option: - `return_zero`: works like true - `return_non_zero`: run all the subtasks regardless theirs exit status, but returns non-zero exit status if any of subtasks failed

… exit code

…at-n#36) - Parse script task content as python using ast - Add support for arg types, including special behaviour for boolean type - Add documentation for named arguments - Improve error handling when parsing CLI arguments Testing & quality - improve organisation of test fixtures - reorganise tests by - add extensive tests for named arguments on script tasks - improve use of type anotations like Mapping Co-authored-by: NEXTHINK\mjaquier <michael.jaquier@nexthink.com> Co-authored-by: Nat Noordanus <n@natn.me>

ThatXliner · 2021-12-24T17:42:56Z

Argh I dislike git

ThatXliner · 2021-12-24T17:44:35Z

I'm going to close this:

I hate git.
I like your progress on the other branch

✨ Initial implementation of Poetry plugin

bc2985e

ThatXliner marked this pull request as draft July 3, 2021 22:10

ThatXliner marked this pull request as ready for review July 3, 2021 22:10

🐛 Should work now

c5e0a0b

🚩 Poetry is now an optional dependancy

1b61517

ThatXliner force-pushed the poetry-plugin branch from 158c1e7 to 1b61517 Compare July 4, 2021 22:55

✨ Pipe to Cleo's stdout and stderr

cbc9799

ThatXliner force-pushed the poetry-plugin branch from defafbd to 1b61517 Compare July 4, 2021 23:14

💡 🎨 Update comments

4f25062

ThatXliner changed the base branch from main to development September 26, 2021 21:32

ThatXliner added 3 commits September 26, 2021 14:38

Merge branch 'development' into poetry-plugin

d8d6cf7

Fixed lockfile

c516cd4

Fix linter errors

de5c4ef

nat-n force-pushed the development branch 7 times, most recently from ee1a1d7 to 5b9f257 Compare November 14, 2021 20:18

nat-n reviewed Dec 21, 2021

View reviewed changes

pyproject.toml Outdated Show resolved Hide resolved

nat-n reviewed Dec 21, 2021

View reviewed changes

Update pyproject.toml

435ff4a

Co-authored-by: nat <n@natn.me>

nat-n mentioned this pull request Dec 23, 2021

Add poetry plugin #46

Merged

ThatXliner and others added 5 commits December 23, 2021 21:22

Some changes

dcb5f4b

Bump version to 0.11.0b3 for pre-release of ignore_fail with non-zero…

dd88c6b

… exit code

rebased?

a8b8f94

ThatXliner force-pushed the poetry-plugin branch from 72a2ccc to a8b8f94 Compare December 24, 2021 17:43

ThatXliner closed this Dec 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial implementation of Poetry plugin #32

Initial implementation of Poetry plugin #32

ThatXliner commented Jul 3, 2021

ThatXliner commented Jul 3, 2021 •

edited

Loading

nat-n commented Jul 4, 2021 •

edited

Loading

ThatXliner commented Jul 4, 2021 •

edited

Loading

ThatXliner commented Jul 4, 2021

ThatXliner commented Aug 21, 2021 •

edited

Loading

nat-n commented Aug 21, 2021

ThatXliner commented Sep 24, 2021

nat-n commented Sep 26, 2021

nat-n commented Sep 26, 2021

nat-n left a comment

nat-n Dec 21, 2021

ThatXliner Dec 23, 2021 •

edited

Loading

nat-n Dec 23, 2021

nat-n Dec 21, 2021

ThatXliner Dec 23, 2021

nat-n Dec 21, 2021

ThatXliner Dec 23, 2021 •

edited

Loading

nat-n Dec 21, 2021

ThatXliner Dec 23, 2021 •

edited

Loading

nat-n Dec 21, 2021

ThatXliner Dec 23, 2021

nat-n Dec 21, 2021

ThatXliner Dec 23, 2021

nat-n commented Dec 23, 2021 •

edited

Loading

ThatXliner commented Dec 24, 2021

ThatXliner commented Dec 24, 2021

ThatXliner commented Dec 24, 2021 •

edited

Loading



		def run_poe_task(name: str, io) -> int:
		io.fileno = sys.stdin.fileno # Monkey patches io because subprocess needs it

Initial implementation of Poetry plugin #32

Initial implementation of Poetry plugin #32

Conversation

ThatXliner commented Jul 3, 2021

ThatXliner commented Jul 3, 2021 • edited Loading

nat-n commented Jul 4, 2021 • edited Loading

ThatXliner commented Jul 4, 2021 • edited Loading

ThatXliner commented Jul 4, 2021

ThatXliner commented Aug 21, 2021 • edited Loading

nat-n commented Aug 21, 2021

ThatXliner commented Sep 24, 2021

nat-n commented Sep 26, 2021

nat-n commented Sep 26, 2021

nat-n left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThatXliner Dec 23, 2021 • edited Loading

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

ThatXliner Dec 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThatXliner Dec 23, 2021 • edited Loading

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

nat-n commented Dec 23, 2021 • edited Loading

ThatXliner commented Dec 24, 2021

ThatXliner commented Dec 24, 2021

ThatXliner commented Dec 24, 2021 • edited Loading

ThatXliner commented Jul 3, 2021 •

edited

Loading

nat-n commented Jul 4, 2021 •

edited

Loading

ThatXliner commented Jul 4, 2021 •

edited

Loading

ThatXliner commented Aug 21, 2021 •

edited

Loading

ThatXliner Dec 23, 2021 •

edited

Loading

ThatXliner Dec 23, 2021 •

edited

Loading

ThatXliner Dec 23, 2021 •

edited

Loading

nat-n commented Dec 23, 2021 •

edited

Loading

ThatXliner commented Dec 24, 2021 •

edited

Loading