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 lsp generation for more accurate documentation #742

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

Conversation

dyl10s
Copy link

@dyl10s dyl10s commented Apr 3, 2025

I was poking around the lsp server and found that you guys generate a good bit of the types using the meta model. I noticed that when I re-ran the generation, I got different results than the currently committed file, and some of the documentation had weird indenting/spacing.

My efforts here were to normalize it and make it map more closely to the metaModel.json. I then ran the generation script so that it updated the lsp_generated.go

@dyl10s
Copy link
Author

dyl10s commented Apr 3, 2025

@microsoft-github-policy-service agree

@jakebailey
Copy link
Member

I noticed that when I re-ran the generation, I got different results than the currently committed file

This shouldn't be possible; the script is deterministic. Did you run the scripts in the dir to grab the pinned copy? Are you missing a format step maybe? Note how CI fails.

I'm not sure I like the code added for this, honestly. It's a lot of code to duplicate for all of the places we write things.

@dyl10s
Copy link
Author

dyl10s commented Apr 3, 2025

This shouldn't be possible; the script is deterministic. Did you run the scripts in the dir to grab the pinned copy? Are you missing a format step maybe? Note how CI fails.

I did run the fetchModel script in the directory to grab the pinned copy using node.

The primary fix for this PR was to resolve the issue where the since documentation would be formatted correctly. I ran the initial script with no changes and got a problem where additional comment lines were added in places they shouldn't be (see image). This is why there was a slightly larger refactor.
image

Let me know if there is a specific way I should be running the fetchModel or generate script. I could be missing something here.

*/
function writeDocumentation(doc) {
function writeDocumentation(doc, proposed, since, sinceTags, deprecated) {
Copy link
Member

Choose a reason for hiding this comment

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

This should really just accept a single parameter of a shape that includes all of these props, looking at each of the calls.

Copy link
Author

Choose a reason for hiding this comment

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

I swapped this to an inline object type. I looked in metaModelSchema.mts to see if there was a base type that each of these objects extended but didn't find one. Let me know if you have a better idea here.

@jakebailey
Copy link
Member

Merging #789 and rerunning CI here shows that the formatting is in fact being changed in an unexpected way.

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.

2 participants