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 Lua Intellisense types #9054

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

abhiaagarwal
Copy link

@abhiaagarwal abhiaagarwal commented Mar 12, 2024

Description

The Lua types used for VSCode Intellisense are out of date. I was recently developing a filter and had to resort to reading the codebase manually to figure out what is/isn't available. This draft PR adds a few functions I found that aren't in lua-types. I was unsure about the types of a few functions and their functionality, please feel free to take over this PR and fill in what I couldn't.

Also might be worth generating these at build-time rather than annotating these manually.

The documentation on the website also needs to be updated. For example, it referenced an Version function which doesn't exist, what does exist is version()

Checklist

I have (if applicable):

@cscheid
Copy link
Collaborator

cscheid commented Mar 12, 2024

Thank you so much! Most of these look good but some of them are not documented on purpose (because we haven't decided on a stable API yet.) It'll be a couple of days until I can review this properly but please ping me again if I forget.

Also, please make sure to submit that collaborator agreement.

@abhiaagarwal
Copy link
Author

CLA has been signed. Feel free to update the branch as needed, I just quickly whipped together something

@abhiaagarwal
Copy link
Author

@cscheid what email should the CLA be sent to? I tried to send it to the [email protected] linked in the CONTRIBUTING but it bounces. Should it be to posit.co instead?

@cscheid
Copy link
Collaborator

cscheid commented Mar 12, 2024

The email you typed is wrong (you have .net instead of .com)

@abhiaagarwal
Copy link
Author

apologies! sent it over. no clue how i missed that

@cscheid
Copy link
Collaborator

cscheid commented Mar 21, 2024

Hi - thanks for the PR, but I think that a number of these function descriptions need to be improved before we can merge them.

For example,

Returns type information related to pandoc or 
quarto-specific elements

This is not a particularly actionable sentence; readers can't see what type of information this returns, and quarto-specific elements is not a phrase we used elsewhere.

I appreciate the effort, and I understand that you don't have enough information to fill in this documentation in the way we'd prefer to have it.

Still, Quarto is better off with no documentation in entries (which sends the message that good documentation is still missing and needed) than with insufficiently-good documentation that sets the standard for future PRs.

@abhiaagarwal
Copy link
Author

abhiaagarwal commented Mar 21, 2024

I agree it's not ready to merge, I just wanted to have some documentation somewhere that these are the functions that are undocumented. Feel free to take over the branch and fix the docs, that's why I marked it as a draft :) Otherwise, I can spend some time later to do it myself, but I currently don't have the bandwidth.

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