Skip to content

Conversation

@pavelzw
Copy link
Member

@pavelzw pavelzw commented Dec 17, 2025

No description provided.

@codecov
Copy link

codecov bot commented Dec 17, 2025

Codecov Report

❌ Patch coverage is 86.20690% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.39%. Comparing base (0416325) to head (d700e93).

Files with missing lines Patch % Lines
src/datajudge/constraints/base.py 71.42% 2 Missing ⚠️
src/datajudge/db_access.py 91.66% 1 Missing ⚠️
src/datajudge/requirements.py 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #306      +/-   ##
==========================================
+ Coverage   92.27%   92.39%   +0.11%     
==========================================
  Files          18       18              
  Lines        2058     2051       -7     
==========================================
- Hits         1899     1895       -4     
+ Misses        159      156       -3     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@kklein kklein added the ready label Dec 18, 2025
[tool.ty.terminal]
error-on-warning = true
[tool.ty.rules]
invalid-method-override = "ignore"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want this?

@kklein
Copy link
Collaborator

kklein commented Dec 18, 2025

Hi @copilot - please explain why the changes introduced with this PR revolving around the caching of get_factual_value and get_target_value lead to the test failures in CI.

Copy link
Contributor

Copilot AI commented Dec 18, 2025

@kklein I've opened a new pull request, #308, to work on those changes. Once the pull request is ready, I'll request review from you.

@kklein
Copy link
Collaborator

kklein commented Dec 19, 2025

@pavelzw @ivergara It seems to me that the remaining ty complaints are due to it not fully understanding how the methods get_factual_value and get_target_value are overwritten in _setup_caching.

Does one of you have a better idea than adding 4 # type: ignore[missing-argument]?

Unfortunately instance method caching is quite messy.

value_target = self.get_target_value(engine)
# ty can't figure out that this is a method and that self is passed
# as the first argument.
value_factual = self.get_factual_value(engine=engine) # type: ignore[missing-argument]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I find this to be quite dissatisfying :(

@kklein
Copy link
Collaborator

kklein commented Dec 20, 2025

Hi @pavelzw - could you ptal at the changes I've made?

@kklein
Copy link
Collaborator

kklein commented Jan 5, 2026

@pavelzw kind bump :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants