-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Spec for Triggers and Custom Clickable Links #15700
base: main
Are you sure you want to change the base?
Conversation
…osoft/terminal into dev/migrie/s/5916-draft
This comment has been minimized.
This comment has been minimized.
@check-spelling-bot Report🔴 Please reviewSee the 📂 files view or the 📜action log for details. Unrecognized words (1)sustainability Previously acknowledged words that are now absenthomeglyphs keyevent ONLCR OSCBG OSCCT OSCFG OSCRCC OSCSCB OSCSCC OSCWT testdlls Tlgdata xxyyzz :arrow_right:To accept ✔️ these unrecognized words as correct and remove the previously acknowledged and now absent words, run the following commands... in a clone of the git@github.com:microsoft/terminal.git repository curl -s -S -L 'https://raw.githubusercontent.com/check-spelling/check-spelling/v0.0.21/apply.pl' |
perl - 'https://github.com/microsoft/terminal/actions/runs/5536057415/attempts/1' ✏️ Contributor please read thisBy default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.
If the listed items are:
See the 🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉 If the flagged items are 🤯 false positivesIf items relate to a ...
|
`match` also accepts just a single string, to automatically assume default | ||
values for the other properties. For example, the JSON `"match": | ||
"[Ee][Rr][Rr][Oo][Rr]"` is evaluated as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But why do that now? We don't even know yet how the feature will actually work in practice and thus don't really know if such shortcuts would be helpful either. Less ways to specify settings seems generally beneficial to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My gut says that more often than not, I just want to match on a line of text as it is output, so I usually don't care about the rest of the match params. I wanted to make it easier for folks to write that into their settings, for folks who are editing by hand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think it'd be better to have just one way to specify settings, at least initially. But I didn't mean to imply that I'm against it either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah see, I'd probably start with this string version first, then expand out to the object version as we add other properties in successive PRs 😝
* `"anchor"` (_default: `"bottom"`_) : Which part of the viewport to run the match against. | ||
- VsCode has this, but I don't think we really do. I think we can just always say bottom. | ||
* `"offset"` (_default: `0`_) : Run the match starting how many rows from the anchor | ||
* `"length"` (_default: `1`_) : How many rows to include in the match |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean it would match 1 line below the viewport? What does an anchor of "bottom" mean and what are the others?
The spec in its entirety might be difficult / time-consuming to implement correctly. Our current URL regex code is still essentially a prototype and would need to be rewritten to work well. For instance, it still doesn't work well with text that is partially outside of the viewport. There's also the problem that we won't be able to correctly match newlines as written in the spec because we currently don't store any newlines. The same would be true for matching trailing whitespace. I feel like we should separate the parts of the spec that we want to implement and ship immediately from the parts that we'd do over time and put those into one or more "Future" section or similar. For instance, the spec says
and we could do that as well initially and forego parts of the settings object and logic needed. |
Oh in my prototype I did some real dumb stuff: |
TODO