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

Infer from type statements #164

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

EliTheGingerCat
Copy link

@EliTheGingerCat EliTheGingerCat commented Jan 5, 2025

I wanted to try contributing to Moonwave and this seemed relatively easy to add. However, my current implementation has everything be on one line, instead of one line per field. I am pretty sure writing the table type explicitly, such as @type {id: number} displays properly, so I am not sure why inferring using Full Moon's string representation does not.

Additionally, I am very new to Rust so feel free to nitpick on style or unidiomatic code. This code copies TypeInfo so that it can later be converted to a string - this sounds unnecessary to me but I could not see any other way to do it without using lifetimes, and considering the rest of the DocEntryKind enum does not use lifetimes, it would feel weird to use it just for the Type variation. Additionally, functions seem to copy strings?

I do not understand why cargo install --path . --locked needs to be run after every change while cargo build will not work (even if I call the executable with an absolute path to target/release). README.md mentions:

You should now be able to change files in the moonwave folder

Maybe I am missing something but I do not see a folder titled "moonwave". Is this just referring to hot reload when you edit Luau files?

Thanks!

Closes #155

@YetAnotherClown
Copy link
Collaborator

Hey, thanks for the PR!

When you follow the development guide, you can edit code in the cli directory and it will live-reload the CLI for Moonwave, you'll have to rerun the CLI commands such as moonwave dev still. You can also edit some code in the docusaurus-plugin-moonwave directory like our React components and that'll work too.

With the extractor, you can just rerun cargo install --path . --locked and then rerun moonwave dev. Also, cargo install --path . --locked works because it installs Moonwave to cargo's bin directory, which lets the CLI use it.

Copy link
Collaborator

@YetAnotherClown YetAnotherClown left a comment

Choose a reason for hiding this comment

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

Just a quick review to guide you in the right direction.

Another thing outside of my review comments is that table types should be the equivalent of an @interface tag, these should be inferred from type statements as well.

Don't forget to also write some test cases in extractor/test-input/ and set them up in extractor/tests/test-inputs.rs. You can run the tests with cargo test. We use insta for snapshot testing, you'll have to read the snapshots from your new tests, confirm everything is correct, and approve the changes.

extractor/src/tags/type_tag.rs Outdated Show resolved Hide resolved
extractor/src/doc_entry.rs Outdated Show resolved Hide resolved
@EliTheGingerCat EliTheGingerCat marked this pull request as ready for review January 12, 2025 20:00
@EliTheGingerCat
Copy link
Author

Using insta was a bit confusing at first, a little tutorial in README.md or at least some mention of it would be nice. I can write it if you want, essentially just cargo test and then cargo insta review. Also, I had to manually install cargo-insta, is this intentional?

@YetAnotherClown
Copy link
Collaborator

cargo-insta is an additional tool that you can use with insta, it's not strictly necessary but it's highly recommended to use.

I also think adding a quick note on snapshot testing to the contributing section would be a great addition to the README.md, feel free to make a new PR for that.

Copy link
Collaborator

@YetAnotherClown YetAnotherClown left a comment

Choose a reason for hiding this comment

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

Looks good so far, we just need to fix some issues that appeared in the snapshot and fix some oversights.

It's mostly just avoiding trivia being included in the snapshot and also accounting for comments for interface fields.

extractor/test-input/passing/type_statement_inference.lua Outdated Show resolved Hide resolved
extractor/src/doc_entry/type_definition.rs Outdated Show resolved Hide resolved
extractor/src/doc_entry/type_definition.rs Outdated Show resolved Hide resolved
extractor/src/doc_entry.rs Outdated Show resolved Hide resolved
extractor/src/doc_entry/type_definition.rs Outdated Show resolved Hide resolved
@EliTheGingerCat
Copy link
Author

Having a bit of trouble with leading trivia being taken as part of lua_type when inferring interfaces. For example:

type interface = {
	field: number --- Comment
}

The lua_type of field would be extracted as number --- Comment with Full Moon's to_string() method.

This is a problem in:

                        doc_entry.fields.push(Field {
                            name,
                            lua_type: field.value().to_string(),
                            desc,
                        });

Is the best course of action to create a function that essentially does the same thing as Full Moon's to_string(), but without trivia? Seems like this is what you suggested for TypeFieldKey.

@YetAnotherClown
Copy link
Collaborator

I've been trying to find a nice way to solve this issue, because your proposed solution would be a lot of work.

TypeFieldKey has only a couple variants, which means its easy to work with it. For the values of Type Fields, they use TypeInfo which has 16 different variants, which would mean a lot of work to do.

Unfortunately, I haven't found a different solution that works.

@EliTheGingerCat
Copy link
Author

EliTheGingerCat commented Jan 26, 2025

I checked snapshots and website preview, everything seems to work EXCEPT for types that use a colon (table and callback) not adding a space after the colon since my TypeInfo stringifier removes all trivia, including spaces. I think this is something the website should handle since, if someone's real Luau code does not have a space after the colon, the website should still be consistent.

An example of the problem:

type petData = baseData & {pet:pet}

On the website, there is no space between "pet" and "pet".

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.

@type comments should auto-fill using the subsequent type definition
2 participants