Skip to content

Make types for selection commands more precise#2304

Merged
robertbrignull merged 1 commit intomainfrom
robertbrignull/selection_types
Apr 13, 2023
Merged

Make types for selection commands more precise#2304
robertbrignull merged 1 commit intomainfrom
robertbrignull/selection_types

Conversation

@robertbrignull
Copy link
Copy Markdown
Contributor

This PR aims to make our types for commands that operate on selections more precise and correct. It's possible for a lot of these commands to receive undefineds that they weren't expecting, and the behaviour is different for different places where the commands are registered.

I've done a lot of manual testing, logging the arguments of commands and triggering them in as many different ways as I could think of. It turns out that tree views behave differently from the file explorer, and title menus also behave differently from context menus. I've split the types in to the various cases and tried to summarise all my findings in the comments.

(btw, I know the type names are quite long. Any suggestions for better names are welcome.)

I then went and corrected types throughout the application. Thankfully we were actually handling undefined values everywhere, which just shows the types were incorrect and giving a false sense of security. The types now match the behaviour of the code and should avoid future problems.

Side-note, this sort of precise typing wouldn't be possible if we hadn't split our commands so they're only called from one place.

Checklist

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
  • Issues have been created for any UI or other user-facing changes made by this pull request.
  • [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

@robertbrignull robertbrignull requested a review from a team April 12, 2023 15:01
@robertbrignull robertbrignull requested a review from a team as a code owner April 12, 2023 15:01
Copy link
Copy Markdown
Contributor

@charisk charisk left a comment

Choose a reason for hiding this comment

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

Thanks for thinking through this, testing the different cases and writing helpful comments!

@robertbrignull robertbrignull merged commit 089d356 into main Apr 13, 2023
@robertbrignull robertbrignull deleted the robertbrignull/selection_types branch April 13, 2023 09:45
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.

2 participants