Repository navigation
Fix easy baseline errors (1/?) - #8081
Conversation
Seeing as I am not Claude................. |
cweider
left a comment
There was a problem hiding this comment.
Some of these changes are more involved than others! Here are initial comments on the straight-forward ones.
e3e5bd5 to
08f19c4
Compare
- Add all non-test modules to type checking - Update baseline - Update CLAUDE.md with new guidance
search/forms.py
`FederalCourtsQuerySet` and `StateCourtQuerySet` were only used once by calling the `as_manager` method, which erased their methods from pyrefly. Just make them managers to fix a bunch of errors
cl.corpus_importer.import_columbia That was certainly an experience
Mostly move things around to eliminate unnecessary `MutableMapping.update` calls. Also add some `TypedDict`s where they help.
`AudioTranscriptionMetadata` save
d2a61ed to
b879211
Compare
|
@cweider Addressed your pagination comment and got confirmation from the team that the Columbia import stuff can just be deleted so no need to mess around. |
cweider
left a comment
There was a problem hiding this comment.
Just a few things to doublecheck. If they don’t need revising, then this is good to go.
|
|
||
| def all_pacer_courts(self) -> "models.QuerySet[Court]": | ||
| return ( | ||
| self.get_queryset() |
There was a problem hiding this comment.
All others use super().get_queryset() Is this intentional? I worry that the super/self distinction is setting things up for trouble down the line. If intentional, probably worth a comment.
I wonder if it’s done for optimization? I suspect that it’s probably not optimizing much. But this is only my take from reading – it’s gotten less attention from me than it has from you.
There was a problem hiding this comment.
There's a minimized Claude review earlier in the PR that brings up the reason why I switched the others to use super(). Originally I had everything using self and just didn't bother to change this one when fixing the other methods.
| Court.FEDERAL_BANKRUPTCY, | ||
| Court.FEDERAL_APPELLATE, | ||
| ] | ||
| class FederalCourtsManager(models.Manager): |
There was a problem hiding this comment.
Considered models.Manager["Court"]?
There was a problem hiding this comment.
In light of the other QuerySet subclasses, I wonder if, perhaps, the problem with the base implementation was the unspecified the generic for models.QuerySet. If done, Self could be used for all of the method return types and it would cut down substantially on the wordiness.
I don’t feel qualified to speak on which is better, a subclass of Manager or QuerySet - do whatever makes sense.
There was a problem hiding this comment.
Pyrefly currently erases extra methods when you call QuerySet.as_manager, which is what this was trying to solve.
| Court.FEDERAL_APPELLATE, | ||
| ] | ||
| class FederalCourtsManager(models.Manager): | ||
| def get_queryset(self) -> "models.QuerySet[Court]": |
There was a problem hiding this comment.
Perhaps models.QuerySet["Court"] (quotes in the generic). I think this is purely aesthetic tho.
| verbose_name_plural = "Courthouses" | ||
|
|
||
|
|
||
| class ClusterCitationQuerySet(models.query.QuerySet): |
There was a problem hiding this comment.
It might be good to give ClusterCitationQuerySet the same treatment as FederalCourtsQuerySet.
| ] | ||
|
|
||
|
|
||
| class OpinionQuerySet(models.QuerySet): |
There was a problem hiding this comment.
It might be good to give OpinionQuerySet the same treatment as FederalCourtsQuerySet.
| Court.FEDERAL_BANKRUPTCY, | ||
| Court.FEDERAL_APPELLATE, | ||
| ] | ||
| class FederalCourtsManager(models.Manager): |
There was a problem hiding this comment.
In light of the other QuerySet subclasses, I wonder if, perhaps, the problem with the base implementation was the unspecified the generic for models.QuerySet. If done, Self could be used for all of the method return types and it would cut down substantially on the wordiness.
I don’t feel qualified to speak on which is better, a subclass of Manager or QuerySet - do whatever makes sense.
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
@cweider Gave it a second thought and decided to leave the |
Fixes
Decreases baseline type errors from 1,294->1,029.
Summary
dictinitializations and addsTypedDicts to avoid various errors caused by improperly inferred types for dicts and itemsoverloadtofind_docket_objectso it's correctly typedFederalCourtsQuerySetandStateCourtsQuerySettoManagers since that's how they were being used anywayas_str_typeincl/search/forms.pyDeployment
This PR should:
skip-deploy(skips everything below)skip-web-deployskip-celery-deployskip-cronjob-deployskip-daemon-deployAI Disclosure