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

feat: shows attributes description on hover #15

Merged

Conversation

cristianoliveira
Copy link
Contributor

@cristianoliveira cristianoliveira commented Aug 28, 2023

Purpose

Implements the document hover feature.

Screenshot 2023-08-28 at 15 17 06

@@ -60,6 +81,30 @@ fn handle_didChange(noti: Notification) -> Option<HtmxResult> {
return None;
}

#[allow(non_snake_case)]
fn handle_didOpen(noti: Notification) -> Option<HtmxResult> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since this feature doesn't require editing the document, it needs to read the current document and store once it is opened

@@ -83,7 +128,10 @@ fn handle_completion(req: Request) -> Option<HtmxResult> {
}
};

error!("handled result: {:?}: completion result: {:?}", completion.context, items);
Copy link
Contributor Author

@cristianoliveira cristianoliveira Aug 28, 2023

Choose a reason for hiding this comment

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

I have auto-format on save 😅

@ThePrimeagen
Copy link
Owner

my man! this has been huge amounts of contributions! if you wish to be invited to collaborators please let me know :)

@@ -26,16 +26,37 @@ struct TextDocumentChanges {
content_changes: Vec<Text>,
}

#[derive(serde::Deserialize, Debug)]
struct TextDocumentOpened {
Copy link
Owner

Choose a reason for hiding this comment

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

i don't know how to use the lsp-types properly yet, but we should try to get rid of these in the future (todo in a pr, don't worry)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean implementing custom structs to parse the requests? or incomplete attributes?

.expect("Why it can't get HX_TAGS?")
.into_iter()
.find(|x| x.name == name)?,
_ => {
Copy link
Owner

Choose a reason for hiding this comment

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

does this trigger if you just hover on a non attribute name / value? wont this just crash the server the moment you hover elsewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense! You mean when hovering a non-htmx attribute, I'll remove the panic

lsp/src/lib.rs Outdated
@@ -77,9 +78,34 @@ fn main_loop(connection: Connection, params: serde_json::Value) -> Result<()> {
error: None,
}))
}

Some(HtmxResult::AttributeHover(h)) => {
Copy link
Owner

Choose a reason for hiding this comment

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

at some point we may want to make this less crae crae

but for now, i am fine with this large ass function ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was under the impression you were avoiding early abstractions hehe. That's why I didn't move it to a module, but it makes sense starting to collocate some of this functionality

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually moving the the bit related to htmx to the htmx module makes so much sense and only have lsp related stuff in /lib.rs I moved :)

@ThePrimeagen
Copy link
Owner

just answer the question abeut the _ => todo!

always worry about those because it may take down our server :(

@cristianoliveira
Copy link
Contributor Author

cristianoliveira commented Aug 28, 2023

my man! this has been huge amounts of contributions!

Thanks, man! I got pretty excited to work with this kind of tooling and I wanted to brush up my Rust skills a bit 😅

if you wish to be invited to collaborators please let me know :)

I would love to! I'm not sure I can contribute as much as in the past days, but for sure I like the idea of coding some Rust from time to time in a project like this :)

lsp/src/lib.rs Outdated Show resolved Hide resolved
@cristianoliveira
Copy link
Contributor Author

cristianoliveira commented Aug 28, 2023

just answer the question abeut the _ => todo!

I figured there was no need to have the default for the pattern match since there were only 2 options, I wonder why rust_analyzer wasn't complaining about this 🤔

@ThePrimeagen
Copy link
Owner

i am going to probably merge this now :)

@ThePrimeagen
Copy link
Owner

this overall looks nice, i am going to accept it :)

@ThePrimeagen ThePrimeagen merged commit 5e96e39 into ThePrimeagen:master Sep 5, 2023
6 checks passed
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