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

refactor(step-generation): getTrashBinAddressableAreaName to always r… #17741

Closed
wants to merge 3 commits into from

Conversation

jerader
Copy link
Collaborator

@jerader jerader commented Mar 12, 2025

…eturn AddressableAreaName

Overview

Slight refactor so that the getTrashBinAddressableAreaName util always returns AddressableAreaName and never null to help with py interop

Test Plan and Hands on Testing

Review the code. functionality should remain the same

Changelog

  • refactor getTrashBinAddressableAreaName to not return null
  • refactor affected usages of the util to grab the trash location in the parent component

Risk assessment

low, should maintain current functionality

@jerader jerader requested a review from a team as a code owner March 12, 2025 19:58
Copy link

codecov bot commented Mar 12, 2025

Codecov Report

Attention: Patch coverage is 58.49057% with 22 lines in your changes missing coverage. Please review.

Project coverage is 61.93%. Comparing base (e792062) to head (21473db).
Report is 30 commits behind head on edge.

Files with missing lines Patch % Lines
...p-generation/src/utils/movableTrashCommandsUtil.ts 50.00% 15 Missing ⚠️
step-generation/src/utils/misc.ts 50.00% 4 Missing ⚠️
...ion/src/commandCreators/compound/dropTipInTrash.ts 62.50% 3 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #17741      +/-   ##
==========================================
- Coverage   61.93%   61.93%   -0.01%     
==========================================
  Files        2824     2823       -1     
  Lines      217106   217121      +15     
  Branches    18437    18439       +2     
==========================================
+ Hits       134458   134465       +7     
- Misses      82463    82471       +8     
  Partials      185      185              
Flag Coverage Δ
protocol-designer 18.84% <30.18%> (-0.01%) ⬇️
step-generation 4.35% <45.28%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...c/timelineMiddleware/generateRobotStateTimeline.ts 87.38% <100.00%> (+0.47%) ⬆️
...ion/src/commandCreators/compound/dropTipInTrash.ts 92.68% <62.50%> (-4.62%) ⬇️
step-generation/src/utils/misc.ts 88.40% <50.00%> (+0.20%) ⬆️
...p-generation/src/utils/movableTrashCommandsUtil.ts 67.59% <50.00%> (-3.69%) ⬇️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@jerader jerader requested a review from ddcc4 March 12, 2025 20:11
if (trashLocation == null) {
console.error(
`could not find trashLocation in airGapInMovableTrash with entity ${trash?.name}`
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey, thanks for doing this, but I'm not sure if it helps much after seeing this code. I was hoping that we'd be able to find a way to skip the == null check entirely. I don't think this change makes the code any simpler or shorter.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah i agree... we shouldn't see trashLocation == null so i could remove the console.error but i don't feel comfortable doing that until we reexamine fixing the location type

@jerader
Copy link
Collaborator Author

jerader commented Mar 14, 2025

closing this PR in place of this: #17774

@jerader jerader closed this Mar 14, 2025
@jerader jerader deleted the sg_trash-addressable-area-util branch March 14, 2025 19:05
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