Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update classifier code for mypy #2122

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

jonathangreen
Copy link
Member

Description

  • Remove all star imports for classifier code from palace.manager.core.classifier import *
    • These were the last star imports we had 🎉
  • Add a __getattr__ method to classifier/__init__.py to let mypy know to expect dynamic imports
  • Move around some of the classifier code to break import cycle
  • Remove duplicated GradeLevelClassifier class

Motivation and Context

  • Trying to get to a place where we can toggle on check_untyped_defs mypy option.

How Has This Been Tested?

  • Running in CI

Checklist

  • I have updated the documentation accordingly.
  • All new and existing tests passed.

@@ -316,139 +311,6 @@ def and_up(cls, young, keyword):
return old


class GradeLevelClassifier(Classifier):
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class was duplicated, and this copy wasn't being used, so I deleted it.

# TODO: Eventually I'd like to refactor this, so we don't have to use __getattr__
# here, and can just import the GenreData objects directly.
def __getattr__(name: str) -> GenreData:
return genres_by_variable_name[name]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the core of the change in this PR.

@@ -821,710 +694,3 @@ def __new__(cls, value):
def scrub_identifier(cls, identifier):
if not identifier:
return identifier


class AgeOrGradeClassifier(Classifier):
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These classes were just moved out to age.py, to break an import cycle.

subject2 = data.transaction.subject(type="type2", identifier="subject2")
subject2.target_age = NumericRange(6, 8, "[)")
subject2.weight_as_indicator_of_target_age = 1
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

weight_as_indicator_of_target_age isn't a property that is defined, and wasn't necessary to set for the test, so I just removed these properties.

@jonathangreen jonathangreen requested a review from a team October 18, 2024 18:18
Copy link

codecov bot commented Oct 18, 2024

Codecov Report

Attention: Patch coverage is 87.33509% with 48 lines in your changes missing coverage. Please review.

Project coverage is 90.81%. Comparing base (801f8af) to head (fe56463).

Files with missing lines Patch % Lines
src/palace/manager/core/classifier/work.py 85.96% 29 Missing and 11 partials ⚠️
src/palace/manager/core/classifier/age.py 87.75% 3 Missing and 3 partials ⚠️
src/palace/manager/core/classifier/gutenberg.py 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2122      +/-   ##
==========================================
+ Coverage   90.73%   90.81%   +0.07%     
==========================================
  Files         351      352       +1     
  Lines       40904    40850      -54     
  Branches     8874     8850      -24     
==========================================
- Hits        37115    37098      -17     
+ Misses       2481     2444      -37     
  Partials     1308     1308              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jonathangreen
Copy link
Member Author

Codecov is complaining, but any code that isn't covered by this, wasn't covered before, so I think this is fine for review without adding more coverage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant