style(Typing): Use stronger types#2714
Conversation
paulglx
force-pushed
the
chore/stronger-types
branch
2 times, most recently
from
91fbe52 to
646f865
Compare
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.
Once credits are available, comment @claude review on this pull request to trigger a review.
wayfarer3130
left a comment
wayfarer3130
left a comment
There was a problem hiding this comment.
I'm ok with the type improvements.
@jbocce - why don't you take a look as well, but I think it is generally in the right direction.
| } | ||
| const preset: ViewportPreset | undefined = | ||
| typeof presetNameOrObj === 'string' | ||
| ? VIEWPORT_PRESETS.find((p) => p.name === presetNameOrObj) |
There was a problem hiding this comment.
Let's use vpPreset or viewportPreset instead.
There was a problem hiding this comment.
Pardon me, I meant the p should be renamed. So as is this case now please rename p to preset.
There was a problem hiding this comment.
Sorry for the confusion! I amended the commit
paulglx
force-pushed
the
chore/stronger-types
branch
from
986b213 to
2cd5352
Compare
| public getProperties = (volumeId?: string): VolumeViewportProperties => { | ||
| public getProperties = ( | ||
| volumeId?: string | ||
| ): VolumeViewportProperties | undefined | null => { |
There was a problem hiding this comment.
We don't return null in this funtion I don't think, so remove the | null.
There was a problem hiding this comment.
It does return null lines 1326-1328 :
const volume = cache.getVolume(volumeId);
if (!volume) {
return null;
}It's the only occurence, we could make it more cohesive by returning undefined instead (bare return;)
There was a problem hiding this comment.
Sorry about that. Let's leave it as you have it.
There was a problem hiding this comment.
No worries, please tell me if you require any other change!
|
|
||
| function volumeNewImageHandler(cameraEvent) { | ||
| function volumeNewImageHandler( | ||
| cameraEvent: EventTypes.CameraModifiedEvent |
There was a problem hiding this comment.
Why not use cameraEvent:Event instead and just narrow once on line 207? I think I prefer that over three casts.
There was a problem hiding this comment.
Agreed, I amended the commit
There was a problem hiding this comment.
Please do not change history by doing either an amend or rebase or anything else during PR. We prefer to see the exact sequence of commits and when they are done.
There was a problem hiding this comment.
Sorry, next time I'll do it sequentially!
paulglx
force-pushed
the
chore/stronger-types
branch
from
2cd5352 to
676847a
Compare
jbocce
left a comment
jbocce
left a comment
There was a problem hiding this comment.
Looks good. Thanks for this change.
3 participants
Context
This PR adds stronger types at three locations in the codebase :
BaseVolumeViewport.tsSynchronizer.tsISynchronizerEventHandler.tsThis enhancement comes from issues in our codebase using Cornerstone3D, where some typing quirks cause us to re-annotate some objects
asthe proper type.Changes & Results
Testing
Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals.
Tested Environment