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

Convert type_id from String to a node #592

Merged
merged 3 commits into from
May 2, 2023

Conversation

jansul
Copy link
Contributor

@jansul jansul commented May 1, 2023

This PR modifies the type_id/type_id! methods of the parser to return a new node (Ast::TypeId) rather than return a String. Any other node affected by this has also been updated.

This should make it easier to implement language server features where it can be useful to exact positions of some nodes. e.g. the name of an Ast::Component

I've tested this change against the mint-realworld repository, running mint docs etc and everything seemed OK, but could possible use more testing given how much is changed.

Copy link
Member

@gdotdesign gdotdesign 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 👍 I guess if it compiles and works with the real word repo it should be good! Once merged I'll test mint-ui with it.

src/ls/hover.cr Outdated Show resolved Hide resolved
Copy link
Member

@Sija Sija left a comment

Choose a reason for hiding this comment

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

I'm wondering, wouldn't be cleaner to implement TypeId#to_s and Comparable(String) to avoid explicit usage of #value property?

src/parsers/html_body.cr Outdated Show resolved Hide resolved
src/type_checkers/use.cr Show resolved Hide resolved
@gdotdesign
Copy link
Member

I'm wondering, wouldn't be cleaner to implement TypeId#to_s and Comparable(String) to avoid explicit usage of #value property?

I guess we could do that, however we currently don't, so I don't know if it's in the scope of this PR or not 🤔

@Sija
Copy link
Member

Sija commented May 1, 2023

@gdotdesign I'm pretty sure we could at least implement TypeId#==(other: TypeId) to avoid doin' in manually everywhere, like:

ast.stores.find(&.name.value.==(connect.store.value))

@jansul
Copy link
Contributor Author

jansul commented May 1, 2023

I was very tempted to implement those methods, however as it's not something we are doing on any other node (especially with Ast::Variable where we also use .value) I thought best to do it like this for consistency.

Happy to make any changes 😄 TypeId#==(other: TypeId) would probably be the safest and least confusing.

Just noticed a bug where I'm not using .value when passing to errors so I will fix that too ...!
image

@jansul
Copy link
Contributor Author

jansul commented May 1, 2023

Fixed all of the above 😄

As an aside it may be worth hardening the bold method of BlockBuilder to only accept strings the same as text as that would have caught that issue!

@gdotdesign gdotdesign merged commit 3f4cbcf into mint-lang:master May 2, 2023
@Sija Sija added this to the 0.18.0 milestone May 2, 2023
@Sija Sija added the refactor label May 2, 2023
@jansul jansul mentioned this pull request May 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

3 participants