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

Update the spec for Action IDs #16816

Merged
merged 5 commits into from
Mar 9, 2024
Merged

Update the spec for Action IDs #16816

merged 5 commits into from
Mar 9, 2024

Conversation

PankajBhojwani
Copy link
Contributor

Summary of the Pull Request

As we start to work on implementing Action IDs, the spec written a few years ago needs some updates. This PR makes those updates for the current implementation plan.

References and Relevant Issues

#6899

PR Checklist

  • Closes #xxx
  • Tests added/passed
  • Documentation updated
    • If checked, please file a pull request on our docs repo and link it here: #xxx
  • Schema updated (if necessary)

This comment has been minimized.

This was referenced Mar 5, 2024
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

Yep this is pretty much just what we discussed last week. Thanks for updating this!

Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

LETS GO

@DHowett DHowett merged commit 7f02b25 into main Mar 9, 2024
12 checks passed
@DHowett DHowett deleted the dev/pabhoj/action_id_spec branch March 9, 2024 19:11
microsoft-github-policy-service bot pushed a commit that referenced this pull request Mar 19, 2024
## Summary of the Pull Request
As outlined in #16816 , adding `OriginTag` to `Command` is one of the
prerequisites to implementing Action IDs. This PR does that.

## Validation Steps Performed
Actions/Commands still get parsed and work

## PR Checklist
- [ ] Closes #xxx
- [ ] Tests added/passed
- [ ] Documentation updated
- If checked, please file a pull request on [our docs
repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx
- [ ] Schema updated (if necessary)
github-merge-queue bot pushed a commit that referenced this pull request Apr 18, 2024
As laid out in #16816, adds an `ID` field to `Command`.

**This first PR only adds IDs for built-in commands in defaults, and
generates IDs for user-created commands that don't define an ID.** Also
note that for now we **will not** be allowing IDs for iterable/nested
commands.

The follow-up PR is where we will actually use the IDs by referring to
commands with them.

Refs #16816 

## Validation Steps Performed
User-created commands in the settings file get rewritten with generated
IDs
github-merge-queue bot pushed a commit that referenced this pull request Jun 4, 2024
As outlined in #16816, refactor `ActionMap` to use the new action IDs
added in #16904

## Validation steps performed

- [x] Legacy style commands are parsed correctly (and rewritten to the
new format)
- [x] Actions are still layered correctly and their IDs can be used to
'overwrite' actions in earlier layers
- [x] Keybindings that refer to an ID defined in another layer work
correctly
- [x] User-defined actions without an ID have one generated for them
(and their settings file is edited with it)
- [x] Schema updated

Refs #16816 
Closes #17133
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.

4 participants