poetry-core
poetry-core copied to clipboard
python-poetry
Refactor Ideas
Resolves: python-poetry#
Based on the Sonar-Cloud's recommendation concerning code quality of nested if-structures, I propose some tiny changes to reduce the complexity.
- [ ] Added tests for changed code.
- [ ] Updated documentation for changed code.
Kudos, SonarCloud Quality Gate passed! 
0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells
No Coverage information
1.9% Duplication
![sonarqubecloud[bot] avatar](https://avatars.githubusercontent.com/in/12526?v=4 "Published by a pub.dev verified publisher"){kind=small}
Much of this appears to be in vendored code, I don't expect this project wants to be making changes there. Perhaps you could take these changes upstream, if you feel motivated to do so.
Perhaps you could take these changes upstream, if you feel motivated to do so.
@dimbleby sorry, can you explain me a little bit more? Thx
Most of the changes currently in this MR are in third party code, vendored into this repository. It would make more sense to offer those changes to the original projects.
The changes that you've made to poetry-core's own code are fair game!
@Anselmoo Any chance you'll be able to get back to this? All the changes in the _vendor folder needs to be dropped, and I'd like to reduce the noise so we can get this reviewed.
@Anselmoo Any chance you'll be able to get back to this? All the changes in the
_vendorfolder needs to be dropped, and I'd like to reduce the noise so we can get this reviewed.
@neersighted thx for letting me know. I wasn't sure if that's still interesting.
If I am correct, the vendor code should be untouched and the rest be checked, again? If so, I will take care. thx
@Anselmoo Any chance you'll be able to get back to this? All the changes in the
_vendorfolder needs to be dropped, and I'd like to reduce the noise so we can get this reviewed.@neersighted thx for letting me know. I wasn't sure if that's still interesting.
If I am correct, the vendor code should be untouched and the rest be checked, again? If so, I will take care. thx
That is correct!
Sorry, @neersighted ~~it seems to be that the rebase of my PR to the current main wasn't successful.~~
Looks better!
It might a good idea to add mypy-module to https://github.com/python-poetry/poetry-core/blob/main/.pre-commit-config.yaml ??
In case of a permanent check of possible refactors, you can actually use https://github.com/sourcery-ai/sourcery as part of the CI/CD.
ignore: ["src/poetry/core/_vendor"]
refactor:
skip: ["extract-method"]
python_version: '3.7'
The changes in e932cdab157189088b357849f080850d5e457feb make things worse, not better.
The type signatures were correct before, and scattering Any all over the place simply makes them less useful.
Edit: I see that most of them relate to NotImplemented. This is a special case that mypy was capable of understanding before the refactoring.
The changes in e932cda make things worse, not better.
The type signatures were correct before, and scattering
Anyall over the place simply makes them less useful.Edit: I see that most of them relate to
NotImplemented. This is a special case thatmypywas capable of understanding before the refactoring.
That's exactly the point, I just wanna write it:
https://github.com/python-poetry/poetry-core/blob/3bf7ad0f1dc2b1a5cabe4bb58cf7e1c6d832870d/src/poetry/core/packages/dependency_group.py#L47-L53
Unfortunately, NotImplemented is type of Any and types.NotImplementedType is only available for python >=3.10.
https://docs.python.org/3/library/types.html#types.NotImplementedType
I don't understand your point. That's exactly the current implementation on master, and it typechecks just fine without an Any annotation?
I don't understand your point. That's exactly the current implementation on master, and it typechecks just fine without an
Anyannotation?
Indeed, it's a little bit wired ...
https://github.com/python-poetry/poetry-core/pull/279/commits/6e3cf6fc039cc3c88edd06e3e674a0cde65f5630
I was running in poetry-main, in code spaces, and in the poetry shell, it's also fine. I am wondering, why the previous test fails?
As I say, I think that mypy has a special case for NotImplemented and the refactoring has disturbed the balance.
Suggest just put that code back as it was and don't worry about it.
(While recognising that such things can be a matter of taste: to my mind the rearrangements in __eq__ etc aren't even an improvement!)
As I say, I think that mypy has a special case for
NotImplementedand the refactoring has disturbed the balance.Suggest just put that code back as it was and don't worry about it.
(While recognising that such things can be a matter of taste: to my mind the rearrangements in
__eq__etc aren't even an improvement!)
Ic, thx @dimbleby for the clarification!
It took me a little bit too long to get your point, sorry
Kudos, SonarCloud Quality Gate passed!
0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells
No Coverage information
No Duplication information
Since it's been almost a year since this PR was last touched and the list of conflicts is quite extensive, I am going to close it. If anyone wants to pick up some of the changes from this PR, feel free to do so and open new PR.
