-
-
Notifications
You must be signed in to change notification settings - Fork 223
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
Evaluate as command #2684
base: dev
Are you sure you want to change the base?
Evaluate as command #2684
Conversation
✅ Deploy Preview for calva-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
CI fails when I add the new file webview.ts, so to work around that I'll just add the webview to snippets
So sweet! Thanks for working on this! 🙏 |
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.
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 think adding the command reference documentation provides a small amount of value:
- Groups commands into logical topics
- Provides another path for discovering what commands are available
- Indexable by search engines
For users like myself who are discovering functionality, I think it is slightly more inviting to have the grouped topics instead of having to read them all in a seemingly random order.
Because it is generated, the maintenance cost is low.
The motivation I had for adding it was that the existing documentation asked for someone to add it 😄
If we don't want it then I think we should update the documentation to remove the previous confusing message.
"category": "Calva", | ||
"command": "calva.info", | ||
"title": "Show Information Message" | ||
}, | ||
{ | ||
"category": "Calva", | ||
"command": "calva.warn", | ||
"title": "Show Warning Message" | ||
}, | ||
{ | ||
"category": "Calva", | ||
"command": "calva.error", | ||
"title": "Show Error Message" | ||
}, |
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.
Maybe people can use Joyride to reach functionality like this instead?
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.
You are correct that notification commands could be left out.
My opinion is that we want to enable toolmakers (like Clay) to do useful things by providing snippets that have "resultAsCommand". The question then is what is useful? Creating a Webview is essential for Clay.
Showing notifications is not essential for Clay. It may be useful for showing info messages or errors, but it is not necessary because those can be done via the Webview.
I think notifications would be broadly useful for toolmakers working on things like standard formatters, test runners, schema validators, and debuggers.
I was surprised that there is no notification command available in VSCode.
VSCode has over 3k commands, Calva has about 100 commands, but none of them handle notification AFAIK. This makes sense because why would a user want to notify themselves? But the intention of this change is to enable toolmakers to request a command, and one obvious thing that toolmakers would benefit from is showing a notification.
This brings up the question of should the feature be "resultAsCommand" at all?
A more conservative approach would be to only provide an interface to the functionality that toolmakers need.
Possibly that doesn't include any commands at all.
I believe that embracing the "command" approach feels nicer, more open, and more powerful for creative customization. And that notifications would be a useful part of that approach.
If adding notifications seems problematic, then I'd like to better understand why, and whether we should be limiting the feature to only show a Webview instead of allowing any command.
Are you concerned about adding notification commands because they would become a breaking change to remove? Perhaps something more?
What has changed?
Fixes #2683
My Calva PR Checklist
I have:
dev
branch. (Or have specific reasons to target some other branch.)published
. (Sorry for the nagging.)[Unreleased]
entry inCHANGELOG.md
, linking the issue(s) that the PR is addressing.npm run prettier-format
)npm run eslint
before creating your PR, or runnpm run eslint-watch
to eslint as you go).Ping @PEZ, @bpringe, @corasaurus-hex, @Cyrik