poetry icon indicating copy to clipboard operation
poetry copied to clipboard

new command to support more options

Open juhoautio opened this issue 3 years ago • 11 comments

Adding all the missing options that are asked interactively by the init command.

Also improving & fixing the doc of init command to match.

Question: How to validate the rendering of docs/cli.md (especially the relref)? -> Answered in https://github.com/python-poetry/poetry/pull/5708#discussion_r885083371

Question: Is it on purpose that mypy is not part of the pre-commit hooks? My 1st push failed on CI because of a mypy error because I didn't realize I should've ran it manually. -> Answered in https://github.com/python-poetry/poetry/pull/5710#issuecomment-1146531780

Pull Request Check List

Resolves: #1854

  • [x] Added tests for changed code.
  • [x] Updated documentation for changed code.

juhoautio avatar May 27 '22 23:05 juhoautio

Deploy preview for website ready!

✅ Preview https://website-67l9ix9lp-python-poetry.vercel.app

Built with commit 16caa173ba761b487fab50d44bba57b5c7fc879f. This pull request is being automatically deployed with vercel-action

github-actions[bot] avatar May 28 '22 11:05 github-actions[bot]

Question: Is it on purpose that mypy is not part of the pre-commit hooks? My 1st push failed on CI because of a mypy error because I didn't realize I should've ran it manually.

Yes, it is for now. mypy was part of our pre-commit hooks but we can't run it in the correct environment without duplicating our list of dependencies. We are probably going to set it up as a tox environment and then suggest running tox to trigger pytest and mypy.

neersighted avatar Jun 04 '22 04:06 neersighted

There are some existing commands that inherit the InitCommand. Should those be also detached by extracting shared helpers? Maybe not yet in this PR though.

juhoautio avatar Jun 04 '22 06:06 juhoautio

Also, let's drop the -l short option for --license as it's inconsistent and it was never documented.

Added in commit https://github.com/python-poetry/poetry/pull/5710/commits/7d578a10d3d0f1074f94a48fe3ea90f67ddebfa1.

I would prefer to see the common parts of both commands factored out into shared helpers.

Done in commit https://github.com/python-poetry/poetry/pull/5710/commits/a09c47b38a15c2803c8bec159f857258eac80838.

juhoautio avatar Jun 07 '22 21:06 juhoautio

@neersighted is this ok? or something to improve still?

juhoautio avatar Jun 10 '22 20:06 juhoautio

@neersighted I tried to address the review comments but looks like this PR was forgotten and there are some conflicts to be resolved. Those should be easy to fix, but would it be possible to have a review for the commits that I added since you last checked this?

juhoautio avatar Sep 01 '22 18:09 juhoautio

Hi @juhoautio -- the maintainers are busy people doing this in their spare time. While an occasional ping (especially to those who have already interacted with a PR/issue) is not unwelcome, pinging people who have not expressed interest in an issue or reviewing code, without a prior understanding of what they are interested in helping with, is generally considered rude. Likewise, pinging repeatedly is generally considered rude.

Additionally, Github as a 'request review/re-review' feature that is generally used for this and makes it easier to reason about what reviews are left on your to-do list as a contributor with many priorities.

Most reviewers will not take a look at code that is conflicted and failing tests as non-mergable code must, by its nature, change (often substantially) in order to be accepted. Thus reviewing it is generally a waste of time for all parties involved. I am sorry that I did not comment that here -- hopefully that explanation/expectation helps in the future when you request reviews.

neersighted avatar Sep 11 '22 19:09 neersighted

Sorry! I had not noticed that there's a Re-request review feature, or at least didn't remember. I'll use that from now on.

I'm also very sorry that I ended up using desperate means like pinging another maintainer. In fact I wasn't sure how to best raise my question about what I should be doing next without appearing rude, yet I failed at it. In the end, I now have the guidance that I was looking for. Thanks a lot for that.

juhoautio avatar Sep 12 '22 18:09 juhoautio

The test failure with Tests / macOS / 3.11-dev (pull_request) doesn't seem to be related to my changes. The same failure seems to be happening at least in one other open PR.

juhoautio avatar Sep 12 '22 18:09 juhoautio

Should the existing --dev-dependency option of init be deprecated?


How about having a new option --group-dependency for both init and new? The value format could be GROUP:DEPENDENCY.

e.g. dev:requests:^2.10.0 or dev:requests=2.11.1 or dev:requests.


or just drop it in general as it's going to be pretty niche during new

The ability to set dev dependencies with new is needed for my use case. Or then I would like to have an option for add that allows only modifying pyproject.toml without updating poetry.lock.

juhoautio avatar Sep 18 '22 08:09 juhoautio

I think the best move is to drop the --dev-dependency change from this PR, and then introduce a new PR deprecating --dev-dependency on init and adding --group-dependency using the design you've proposed.

neersighted avatar Sep 18 '22 18:09 neersighted

there is a lot of moving code around in this PR in the same commit it was changed, so it's a bit hard to review.

This is unfortunate, yes. The commit to blame for most of the refactoring is https://github.com/python-poetry/poetry/pull/5710/commits/a09c47b38a15c2803c8bec159f857258eac80838. In that commit I am only moving the existing code to functions but not changing it otherwise.

I think we should block this on #6669 as the new AbstractRepository introduced in that PR will make all the functions that currently take Pool (which just has PyPiRepository in it anyway) much cleaner and allow for dropping Pool until such time that we actually support searching sources other than PyPI.

Looks like that PR was already merged 🎉 I'm sorry but I couldn't figure out what it would mean in practice for this PR, how to replace Pool with AbstractRepository in these functions? I was about to take AbstractRepository in find_best_version_for_package but it ends up using VersionSelector(pool). Is it possible to change the signature of VersionSelector to take AbstractRepository instead of Pool?

I committed something along these lines, curious to hear the feedback (commit https://github.com/python-poetry/poetry/pull/5710/commits/b3e5364a3dbc0748be568372bad362191786c864). But I rolled it back because it seemed to be trying real PyPI calls and made the build fail on Travis. I would also like to omit it to keep this PR simpler.

juhoautio avatar Oct 10 '22 19:10 juhoautio

@juhoautio: Sorry I'm late to the party. I think we should rework the implementation of poetry new and poetry init in that way, that poetry new becomes an extended version of poetry init. Doing this we can makes sure, that new have at least the same options as init, even if we introduce a new option.

This is why I closed the issue you've linked in your PR description. The new "major" ticket should be #2563 now.

finswimmer avatar Oct 21 '22 11:10 finswimmer

@finswimmer is someone (you?) going to work on implementing #2563 some time soon? If yes, does that means this PR is rejected after all? But if no one will work on #2563, I would still hope to have this PR merged with the current scope ie. just adding some missing options & fixing some docs without any bigger refactoring.

juhoautio avatar Oct 22 '22 06:10 juhoautio

Hey @juhoautio ,

this doesn't mean your PR is rejected. The scope is not that far away from #2563 and I was hoping that you maybe like to go that extra mile. :)

fin swimmer

finswimmer avatar Oct 22 '22 07:10 finswimmer

I could work on it if you'd like, but if the design changes that much, I think starting from a clean slate with a new PR could be better. I would hope to have some guidance on the desired approach for implementing it. Please let me know.

I'm still wondering if this PR could be finished because it's almost there. Maybe I'm also missing what additional use cases the new alternative will enable compared to this.

juhoautio avatar Oct 22 '22 09:10 juhoautio

Superseded by #9088

Secrus avatar Mar 03 '24 00:03 Secrus

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[bot] avatar Apr 03 '24 00:04 github-actions[bot]