mypy: tackle all violations in excluded files #1275
Conversation
mergify
bot
added
testing
booxter
force-pushed
the
more-mypy-fixes
branch
3 times, most recently
from
80de93e to
5e173ea
Compare
|
The remaining violations in the file are tackled in #1287 Both PRs combined should allow us to enable mypy for this module. |
|
e2e workflow launched on this PR: View run |
|
e2e workflow succeeded on this PR: View run, congrats! |
|
This pull request has merge conflicts that must be resolved before it can be |
booxter
changed the title
mergify
bot
added
ci-failure
mergify
bot
removed
needs-rebase
booxter
force-pushed
the
more-mypy-fixes
branch
2 times, most recently
from
c187a75 to
a189eb0
Compare
|
I'm collecting all relevant fixes for mypy here now. Seems counterproductive to split these in tiny PRs. |
mergify
bot
added
ci-failure
tiran
left a comment
tiran
left a comment
There was a problem hiding this comment.
The changes are fine.
I still think it is too noisy to land these changes as a 38 separate commits. It's way too noisy in the git history. IMO we don't need fine-grained history for code refactoring. After all these changes should not change behavior.
mergify
bot
added
ci-failure
leseb
left a comment
leseb
left a comment
There was a problem hiding this comment.
@booxter has been very patient with this large change, and the constant rebasing must have been painful. The PR has no conflicts, so I think we should free him now 🙂 It’s been a while already. Honestly, when I look at all the questionable commits that get merged these days, I cringe every time I check the commit history. @booxter did a great job with well-formatted commit messages. I don’t think we should be blocking him on the number of commits. Thanks!
|
I like to add another data point. I haven't mentioned this before, but I did not want to insinuate foul play. As an open source project, we welcome contributions from everybody. In return, we recognize and celebrate the work from our contributors. For some contributors, metrics such as contribution and activity statistics are important. I fear that merging 38 separate commits will skew the statistics and set bad precedence. To put the number in relation: Russel is currently the top contributor with 89 commits in total (main branch, excluding merge commits). Places 9 has 38 commits and place 10 has 33 commits. To be clear, I don't think that the 38 commits in this PR were created out of malicious intent. You are doing a great job. I really want to have your fixes in, so we can improve our typing and code quality even further. I hope my explanation shows that there are not only technical issues (bisect, bloated history, hard to revert), but also a human issue. With this new information, would you be willing to compromise and squash your PR into a smaller number of commits? I'm no longer insisting on one commit, but would prefer something along the line of half a dozen to a dozen commits. Given your technical skills and experience with typing, I would also like to nominate you to become a triager and eventually a maintainer -- if you are interested. I'll have to talk to some people and figure out how to initiate the process. |
Signed-off-by: Ihar Hrachyshka <ihrachys@redhat.com>
It is directly import from utils module, so we should not rely on it being pulled by our other dependencies. Signed-off-by: Ihar Hrachyshka <ihrachys@redhat.com>
This subclass type is more special. Signed-off-by: Ihar Hrachyshka <ihrachys@redhat.com>
Warnings fixed: Item "None" of "LlamaProxy | None" has no attribute "_current_model" [union-attr] Item "None" of "Llama | Any | None" has no attribute "chat_handler" [union-attr] Signed-off-by: Ihar Hrachyshka <ihrachys@redhat.com>
Signed-off-by: Ihar Hrachyshka <ihrachys@redhat.com>
Signed-off-by: Ihar Hrachyshka <ihrachys@redhat.com>
|
@tiran don't get me wrong, I am not frustrated about any particular rule or policy (though of course I have my opinions), I just want to see some clarity as to what is actually considered "the right way" in this project. Signals so far are diametrically different. :) This is a bit frustrating indeed. (Honestly, I didn't think this PR will get so contentious.) I understand people watch stats. Ideally, people don't use these numbers to celebrate or judge others' impact. (Which is e.g. minuscule here.) But if this keeps people happy, so be it. I merged most of mypy patches into one; left unrelated changes outside; only one mypy specific patch is left separated because it's not run-of-the-mill one. Let me know if this is enough. |
6 participants
Next steps will be: adding more type annotations to existing functions; enabling stricter validations (--strict), revisiting existing type annotations and maybe tightening where possible (str -> Path, fewer Unions etc.)