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

onDidAccept proposed APIs don't match naming convention #52979

Closed
Tyriar opened this issue Jun 26, 2018 · 4 comments
Closed

onDidAccept proposed APIs don't match naming convention #52979

Tyriar opened this issue Jun 26, 2018 · 4 comments
Assignees
Labels
*as-designed Described behavior is as designed quick-pick Quick-pick widget issues
Milestone

Comments

@Tyriar
Copy link
Member

Tyriar commented Jun 26, 2018

https://github.com/Microsoft/vscode/wiki/Extension-API-guidelines#event-naming

Maybe they should be onDidAcceptText given #49340 (comment)?

Related: #52882 (comment)

@chrmarti
Copy link
Collaborator

For QuickPick it might have to be onDidAcceptItems because it really fires for all the different 'accept' gestures that should behave consistently for each widget. Needs some more thought.

@chrmarti chrmarti added bug Issue identified by VS Code Team member as probable bug quick-pick Quick-pick widget issues labels Jun 26, 2018
@chrmarti chrmarti added this to the June 2018 milestone Jun 26, 2018
@chrmarti
Copy link
Collaborator

@jrieken I believe we discussed this, did we conclude that onDidAccept was fine? We could use onDidAcceptValue on InputBox and onDidAcceptItems (although often it will be exactly one) on QuickPick. One reason against being specific was that we would let extensions decide what to 'accept', but we later discarded that reasoning because we expect extensions to consistently accept the value in InputBox and the items in QuickPick and use the buttons for alternative actions (like accepting the value in a QuickPick). Opinions?

@jrieken
Copy link
Member

jrieken commented Jun 28, 2018

Yeah, the rule says on[Did|Will]VerbSubject but we have a few places in which we skip the Subject because its obvious from the context, like WebView#onDidDispose (instead of WebView#onDidDisposeWebview). It never hurts to be explicit but if the context/subject is obvious (which is likely to happen more often with "local/private" events) then it is an option to skip the subject-part

@chrmarti
Copy link
Collaborator

Sounds good. I'll keep the existing naming then. That also makes it somewhat consistent across these interfaces.

@chrmarti chrmarti added *as-designed Described behavior is as designed and removed bug Issue identified by VS Code Team member as probable bug labels Jun 28, 2018
@vscodebot vscodebot bot locked and limited conversation to collaborators Aug 12, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
*as-designed Described behavior is as designed quick-pick Quick-pick widget issues
Projects
None yet
Development

No branches or pull requests

3 participants