new command to support more options#5710
new command to support more options#5710juhoautio wants to merge 23 commits intopython-poetry:mainfrom
Conversation
Adding all the missing options that are asked interactively by the init command
|
Deploy preview for website ready! ✅ Preview Built with commit 16caa17. |
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
left a comment
neersighted
left a comment
There was a problem hiding this comment.
I'm not very enthusiastic about subclassing the init command -- instead, I would prefer to see the common parts of both commands factored out into shared helpers. Also, let's drop the -l short option for --license as it's inconsistent and it was never documented.
|
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. |
|
@neersighted is this ok? or something to improve still? |
|
@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? |
|
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. |
|
Sorry! I had not noticed that there's a 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. |
This comment was marked as outdated.
This comment was marked as outdated.
neersighted
left a comment
neersighted
left a comment
There was a problem hiding this comment.
Overall looks good to me, but I'm not sure about the addition of --dev-dependency while we're moving away from --dev. I think we should either generalize it to any group, or just drop it in general as it's going to be pretty niche during new.
|
Should the existing How about having a new option e.g.
The ability to set dev dependencies with |
|
I think the best move is to drop the |
neersighted
left a comment
neersighted
left a comment
There was a problem hiding this comment.
Some more tweaks -- 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.
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.
| return 0 | ||
|
|
||
| def _get_pool(self) -> Pool: | ||
| if isinstance(self, EnvCommand): |
There was a problem hiding this comment.
This check makes no sense given the type of the command.
| return result | ||
|
|
||
|
|
||
| def create_pool() -> Pool: |
There was a problem hiding this comment.
Let's drop this helper and keep things inline -- it's less messy than stashing this helper in such an obscure place, and this isn't the PR to solve the ergonomics of creating the pool in anyway. If you want to clean things up and be a bit more specific, you can just use PyPiRepository.search() instead of creating a pool with one repository.
There was a problem hiding this comment.
If you want to clean things up and be a bit more specific, you can just use PyPiRepository.search() instead of creating a pool with one repository.
I'm all for cleaning things up, but not sure what it means to replace this with PyPiRepository.search() -> moving the code inline. Also, good to avoid additional refactoring in this PR's scope.
This is unfortunate, yes. The commit to blame for most of the refactoring is a09c47b. In that commit I am only moving the existing code to functions but not changing it otherwise.
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 I committed something along these lines, curious to hear the feedback (commit b3e5364). 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. |
|
|
||
| name = package.name | ||
| selector = VersionSelector(self.poetry.pool) | ||
| selector = VersionSelector(self.poetry.pool.repositories[0]) |
There was a problem hiding this comment.
What would be the right way to handle this? Moving from Pool to AbstractRepository.
This reverts commit b3e5364.
juhoautio
force-pushed
the
new_command_args
branch
from
e387994 to
b9ba428
Compare
|
@juhoautio: Sorry I'm late to the party. I think we should rework the implementation of This is why I closed the issue you've linked in your PR description. The new "major" ticket should be #2563 now. |
|
@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. |
|
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 |
|
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. |
|
Superseded by #9088 |
|
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. |
4 participants
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 therelref)?-> Answered in #5708 (comment)
Question: Is it on purpose that
mypyis 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 #5710 (comment)
Pull Request Check List
Resolves: #1854