Skip to content

Count a variable as skipped if its dependencies are skipped#561

Open
takluyver wants to merge 1 commit into
masterfrom
fix/propagate-dep-skip
Open

Count a variable as skipped if its dependencies are skipped#561
takluyver wants to merge 1 commit into
masterfrom
fix/propagate-dep-skip

Conversation

@takluyver

Copy link
Copy Markdown
Member

At present, a dependent variable is recorded as skipped if its dependencies return None, but as having an error if its dependencies raise Skip:

Screenshot From 2026-04-24 16-51-21

With this change, the Skip is propagated instead, making it easier to see which variables actually hit an error:

image

@takluyver takluyver requested a review from tmichela April 24, 2026 15:59
@takluyver takluyver added the bug Something isn't working label Apr 24, 2026
@codecov

codecov Bot commented Apr 24, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.32%. Comparing base (734ab7b) to head (b7fd714).
⚠️ Report is 5 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #561      +/-   ##
==========================================
- Coverage   79.34%   79.32%   -0.02%     
==========================================
  Files          42       42              
  Lines        7659     7658       -1     
==========================================
- Hits         6077     6075       -2     
- Misses       1582     1583       +1     

☔ 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.

Comment on lines +518 to 519
if dep_errors and not all(isinstance(e, Skip) for _, e in dep_errors):
errors[name] = DependencyError(dep_errors)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
if dep_errors and not all(isinstance(e, Skip) for _, e in dep_errors):
errors[name] = DependencyError(dep_errors)
if dep_errors:
errors[name] = DependencyError(dep_errors)
if all(isinstance(e, Skip) for _, e in dep_errors):
errors[name] = Skip(str(errors[name]))

I suggest to keep building the dependency tree of skipped variables, to help understand why it was skipped.

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

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants