Skip to content

Add strict typing to src/palace/manager/api/lanes.py#3023

Merged
dbernstein merged 2 commits intomainfrom
add-type-hints-to-palace-manager-api-lanes
Feb 5, 2026
Merged

Add strict typing to src/palace/manager/api/lanes.py#3023
dbernstein merged 2 commits intomainfrom
add-type-hints-to-palace-manager-api-lanes

Conversation

@dbernstein
Copy link
Contributor

@dbernstein dbernstein commented Feb 4, 2026

Description

Adds type hints to src/palace/manager/api/lanes.py

Motivation and Context

Ongoing effort to bring type hints to the entire code base.

How Has This Been Tested?

Unit tests. Mypy. Very very very minor functional changes that are covered by tests.

Checklist

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

@codecov
Copy link

codecov bot commented Feb 4, 2026

Codecov Report

❌ Patch coverage is 94.44444% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.03%. Comparing base (985e404) to head (bdc530c).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/palace/manager/api/lanes.py 94.44% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3023   +/-   ##
=======================================
  Coverage   93.02%   93.03%           
=======================================
  Files         479      479           
  Lines       43527    43539   +12     
  Branches     6041     6041           
=======================================
+ Hits        40493    40505   +12     
  Misses       1965     1965           
  Partials     1069     1069           

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

@dbernstein dbernstein marked this pull request as ready for review February 4, 2026 17:35
Copy link
Contributor

@tdilauro tdilauro left a comment

Choose a reason for hiding this comment

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

This looks good! 🎸

This is approved, but the "No functional changes" statement in the PR description is not accurate right now (see comment below). Please resolve that comment one way or the other before merging.

Comment on lines +848 to +849
else:
languages = list(languages)
Copy link
Contributor

Choose a reason for hiding this comment

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

After ll.846-849, languages is guaranteed to be a list[str], so it might make sense to use a new variable here with a name like languages_list:

    if isinstance(languages, str):
        languages_list = [languages]
    else:
        languages_list = list(languages)

or just

    languages_list = (
        [languages] 
        if isinstance(languages, str)
        else list(languages)
    )

and then be sure to use languages_list for the rest of the function.

Similar case in create_lane_for_tiny_collection below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will fix.

Comment on lines +937 to +938
else:
languages = list(languages)
Copy link
Contributor

Choose a reason for hiding this comment

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

See comment above in create_lane_for_small_collection.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will fix.

"""
if not languages:
return None
return priority
Copy link
Contributor

Choose a reason for hiding this comment

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

The PR comment says that no functional changes were made, but this looks like a functional change. If you decide to leave this in, please update the PR description, so that it is accurate.

Also, we didn't make any changes to the tests in this PR, so that probably means that this case is not covered.

Copy link
Contributor Author

@dbernstein dbernstein Feb 5, 2026

Choose a reason for hiding this comment

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

The tests already cover checking for a priority = 0. I'll also correct the PR comment.

@dbernstein dbernstein force-pushed the add-type-hints-to-palace-manager-api-lanes branch from a77099e to bdc530c Compare February 5, 2026 18:34
@dbernstein dbernstein enabled auto-merge (squash) February 5, 2026 18:38
@dbernstein dbernstein merged commit c6732a9 into main Feb 5, 2026
19 checks passed
@dbernstein dbernstein deleted the add-type-hints-to-palace-manager-api-lanes branch February 5, 2026 18:42
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.

2 participants