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

[Not Ready] [typespec-vscode] Improvements for the intellisense of tspconfig.yaml #5186

Open
wants to merge 27 commits into
base: main
Choose a base branch
from

Conversation

mzhongl524
Copy link
Member

@mzhongl524 mzhongl524 commented Nov 25, 2024

Add a intellisense for the extends path in tspconfig.yaml

Fixes:#4855

@microsoft-github-policy-service microsoft-github-policy-service bot added the ide Issues for VS, VSCode, Monaco, etc. label Nov 25, 2024
@azure-sdk
Copy link
Collaborator

azure-sdk commented Nov 25, 2024

All changed packages have been documented.

  • @typespec/compiler
Show changes

@typespec/compiler - fix ✏️

Improvements for the intellisense of tspconfig.yaml,> - Support the auto completion for extends, imports, rule, rule sets and variables in tspconfig.yaml,> - Show required/optional information in the details of emitter's options completion item in tspconfig.yaml

@typespec/compiler - fix ✏️

Fix bug in tspconfig.yaml,> - Fix the issue that extra " will be added when auto completing emitter options inside ""

@azure-sdk
Copy link
Collaborator

You can try these changes here

🛝 Playground 🌐 Website 📚 Next docs

@markcowl markcowl assigned chrisradek and RodgeFu and unassigned chrisradek Nov 26, 2024
@mzhongl524
Copy link
Member Author

mzhongl524 commented Nov 27, 2024 via email

@RodgeFu RodgeFu changed the title [typespec-vscode] Improvements for the intellisense of tspconfig.yaml [Not Ready] [typespec-vscode] Improvements for the intellisense of tspconfig.yaml Nov 28, 2024
- The rule or ruleSets of the linter can be auto-completed
- Emitter optoins autocomplete intelligently handles quotation mark display
- Autocomplete of variable interpolation
- The parameters of emitter's options distinguish whether they are required or optional
Copy link
Contributor

Choose a reason for hiding this comment

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

Please create two changelog items, one as new feature for:
Support the auto completion for extends, imports, rule, rule sets and variables in tspconfig.yaml
Show required/optional information in the details of emitter's options completion item in tspconfig.yaml
the other as bug fix for:
Fix the issue that emitter option auto completion would extra " while inside "..."

const variableInterpolationItems = resolveVariableInterpolationCompleteItems(
target.yamlDoc,
target.path,
tspConfigDoc.getText().slice(target.sourceRange?.pos, target.cursorPosition),
Copy link
Contributor

Choose a reason for hiding this comment

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

what would happen if target.sourceRange is undefined?

}
}

// If there is no corresponding ruleSet in the library, add the library name directly.
Copy link
Contributor

Choose a reason for hiding this comment

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

the comment seems no longer valid.

const exports = await pkg.getModuleExports();
if (exports?.$linter?.ruleSets !== undefined) {
// Below ruleSets are objects rather than arrays
for (const [ruleSet] of Object.entries(exports?.$linter?.ruleSets)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I remember Object.entries will throw if undefined is given. please help to confirm. same comment for other places using Object.entries.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, Object.entries will throw, but judgment has ben made before use

Copy link
Contributor

Choose a reason for hiding this comment

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

then please remove the ? so that compiler can help you to catch it in case the pre-check is removed for some reason in the future.

* @param target The target object of the current configuration file, see {@link YamlScalarTarget}
* @returns CompletionItem object
*/
function createContainingQuatedValCompetionItem(
Copy link
Contributor

Choose a reason for hiding this comment

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

how about createCompletionItemWithQuote?

target.cursorPosition <= target.sourceRange.end
) {
// If it is a quoted string, the relative position needs to be reduced by 1
const relativePos =
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not a pos but a length

---

Fix bug in tspconfig.yaml
- Fix the issue that emitter option auto complete while inside "" will add extra ""`
Copy link
Contributor

Choose a reason for hiding this comment

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

Fix the issue that extra " will be added when auto completing emitter options inside ""

tspConfigPosition,
target,
);
if (item) {
Copy link
Contributor

Choose a reason for hiding this comment

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

if we can't create an item with quote, we should fall back to the original way instead of not adding the item

);
if (item) {
items.push(item);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

create in original way if can't create item withQuote. or you can put that logic in the method as a fallback solution

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:core Issues for @typespec/compiler ide Issues for VS, VSCode, Monaco, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants