Skip to content

Comments

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

Merged
radoering merged 8 commits intopython-poetry:masterfrom
abn:use-export-plugin
Apr 16, 2022
Merged

export: replace in-tree implementation with poetry-plugin-export#5413
radoering merged 8 commits intopython-poetry:masterfrom
abn:use-export-plugin

Conversation

@abn
Copy link
Member

@abn abn commented Apr 5, 2022 •
edited
Loading

Once the export plugin code releases 1.x (python-poetry/poetry-plugin-export#46), this PR should add changes to replace the export command.

Closes: #4824
Depends-on: #5412

Notes:

  • Maintain commits when merging.
  • Reviewing per commit might be easier.

@abn abn force-pushed the use-export-plugin branch from afdff9a to 6d270cd Compare April 5, 2022 17:44
@abn abn changed the title Change tracking for adopting new export plugin instead of internal export command export: replace in-tree implementation with poetry-plugin-export Apr 5, 2022
@abn abn requested a review from a team April 5, 2022 17:46
@abn abn marked this pull request as ready for review April 5, 2022 17:50
@abn abn force-pushed the use-export-plugin branch from 6d270cd to aee5e05 Compare April 5, 2022 20:27
@abn abn added area/cli Related to the command line area/plugin-api Related to plugins/plugin API labels Apr 5, 2022
@abn abn added this to the 1.2 milestone Apr 5, 2022
@abn abn mentioned this pull request Apr 6, 2022
Merged
@radoering radoering mentioned this pull request Apr 9, 2022
Merged
radoering
radoering reviewed Apr 9, 2022
View reviewed changes
radoering
radoering requested changes Apr 9, 2022
View reviewed changes
Copy link
Member

@radoering radoering left a comment

Choose a reason for hiding this comment

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

Before merging this PR, there are some issues in poetry-plugin-export that should be fixed. (Its tests ran against the old in-tree implementation!)

See python-poetry/poetry-plugin-export#50 for proposed changes.

However, the poetry-export-plugin PR is blocked by #5435 which should be merged into master first. Then, this branch can be rebased and python-poetry/poetry-plugin-export#50 can be updated to pass the tests.

Further, there is a change in behavior that I am not sure is intentional: The in-tree export command did not export dev dependencies by default. There was even a test for that default behavior. The poetry-plugin-export exports all non-optional dependencies (including dev-depdendencies) by default.

@abn
Copy link
Member Author

abn commented Apr 10, 2022

Thanks @radoering. Ugg; good catches. As for the beaviour change, I commented on the python-poetry/poetry-plugin-export#50. I need to take a closer look at #5435, first glance reminds of #5311 (comment).

@abn abn force-pushed the use-export-plugin branch from aee5e05 to 642775b Compare April 10, 2022 22:32
@abn abn requested a review from radoering April 11, 2022 08:03
radoering
radoering previously requested changes Apr 11, 2022
View reviewed changes
Copy link
Member

@radoering radoering left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I think there two open points that build on each other:

  1. decision on default behavior (including --with/--without handling), see latest changes in python-poetry/poetry-plugin-export#50 for the variant "no breaking change but different behavior of export command compared to install command"
  2. update documentation (should be done after decision on default behavior was made) btw, the documentation has to remain in this repo?

@abn abn force-pushed the use-export-plugin branch from 397b1f0 to 29b03af Compare April 11, 2022 18:50
@abn
Copy link
Member Author

abn commented Apr 11, 2022

poetry-plugin-export==1.0.1 should fix the issues. I have also added an additional test run to verify everything works.

@abn abn requested a review from radoering April 11, 2022 18:56
@radoering
Copy link
Member

radoering commented Apr 12, 2022

2. update documentation (should be done after decision on default behavior was made) btw, the documentation has to remain in this repo?

Still needs to be done.

@abn
Copy link
Member Author

abn commented Apr 12, 2022

Ack, will update.

Until we have a better strategy for handling this, documentation should remain in this repo as the current website publishing only accounts for this repository. But, in the mid-term (outside of this PR), figure out how to get the website to also support any documentation we put in the plugin repository.

The export plugin is a snow flake at the moment, so handling it as is for now is okay I reckon. But if we start adding more "bundled" plugins, we should come up with a better solution sooner.

@abn abn force-pushed the use-export-plugin branch from 29b03af to 2444521 Compare April 12, 2022 19:47
@radoering radoering merged commit dca0c56 into python-poetry:master Apr 16, 2022
@abn abn mentioned this pull request Jun 6, 2022
Merged
@radoering radoering mentioned this pull request Jun 16, 2023
Closed
@github-actions
Copy link

github-actions bot commented Feb 29, 2024

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.

Reviewers

@radoering radoering radoering approved these changes

Assignees

No one assigned

Labels

area/cli Related to the command line area/plugin-api Related to plugins/plugin API

Projects

None yet

Milestone

1.2

Development

Successfully merging this pull request may close these issues.

2 participants

@abn @radoering