Skip to content

Renames for csv overrides PR#248

Merged
AndreasArvidsson merged 17 commits intomasterfrom
sub-token-exclusions-error
Aug 31, 2021
Merged

Renames for csv overrides PR#248
AndreasArvidsson merged 17 commits intomasterfrom
sub-token-exclusions-error

Conversation

@pokey
Copy link
Copy Markdown
Member

@pokey pokey commented Aug 23, 2021

Many changes to support the new names in csv overrides PR. This PR also does the following:

  • Until we properly support them, we throw an exception for subtoken exclusions. Probably not worth supporting properly until we generalize the "next" / "nth" thing in next-gen cursorless engine
  • Adds a script to transform recorded test cases in bulk

Todo:

  • Throw proper error for unknown action from canonicalize
  • Remove old extra actions and use canonicalization instead

Comment thread src/typings/Types.ts Outdated
Comment on lines +116 to +117
includeAnchor?: boolean;
includeActive?: boolean;
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

making these optional for now for backwards compatibility. Also, I went with "include" instead of "exclude" because I prefer positive booleans, otherwise it's a double negative when they're false which is a bit more cognitive load

my one hesitation with using "include" instead of "exclude" is that while these are optional, we'll be defaulting to true, which is always a bit jarring cc/ @AndreasArvidsson

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.

I will always prefer that optional booleans default to false. That why they were defined as excluding.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes but I would prefer to make these not optional. The reason they're optional is for backwards compatibility. Hmm. I'm not seeing a way to make them backwards compatible while making them positive. Any ideas?

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.

In Infer full targets just do.
Includeachor = target.includeanchor ?? true

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Wait isn't that defaulting to true?

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.

Yes and that is how it is today

Comment thread src/canonicalizeActionName.ts
@pokey pokey changed the title Throw error for sub token exclusions Renames for csv overrides PR Aug 30, 2021
@pokey pokey requested a review from AndreasArvidsson August 30, 2021 18:06
@AndreasArvidsson AndreasArvidsson merged commit e15bf2e into master Aug 31, 2021
@AndreasArvidsson AndreasArvidsson deleted the sub-token-exclusions-error branch August 31, 2021 06:19
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