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

fix: issue suggesting in beetween other hx-* tags #16

Conversation

cristianoliveira
Copy link
Contributor

Fixing the issue I mentioned in another PR about suggesting in the middle of other tags, or when there are incomplete tags

demo-suggestion.mov

@cristianoliveira cristianoliveira changed the title fix: issue suggesting middle of tags fix: issue suggesting in beetween other hx-* tags Aug 29, 2023
match_
.captures
.iter()
.filter(|capture| capture.node.start_position() <= trigger_point)
Copy link
Contributor

Choose a reason for hiding this comment

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

if the trigger_point is after the node end position this doesn't work as expected or am I understanding it wrong?

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 changed a bit this approach but you are correct, because the node may end after the trigger point for instance
<div hx-target="|" ... in the previous implementation it needed to understand what was else the node has or for the cases where there was a mismatch of quotes <div hx-foo="| class="bar"> the Errored node was at end, but I changed a bit and now it filters the nodes before the analysis happen

Some(props)
}

pub fn query_attr_keys_for_completion(node: Node<'_>, source: &str) -> Option<Position> {
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 moved the logic of querying for attribute key and name suggestions to their own function because it was getting a bit too complex and long to manage :)

@cristianoliveira cristianoliveira force-pushed the fix/issue-suggesting-middle-of-tags branch from ddb9edc to 14e00e2 Compare August 31, 2023 15:14
@ThePrimeagen
Copy link
Owner

we have a conflict, could you resolve and i'll review

@cristianoliveira cristianoliveira force-pushed the fix/issue-suggesting-middle-of-tags branch from 14e00e2 to 95c6a1c Compare September 6, 2023 06:18
@cristianoliveira
Copy link
Contributor Author

There you go. I also included the fix I mentioned in #17

@cristianoliveira cristianoliveira force-pushed the fix/issue-suggesting-middle-of-tags branch from b197a44 to d0dc066 Compare September 6, 2023 06:31
@ThePrimeagen
Copy link
Owner

ok, let me review and validate :)

"\"".to_string(),
" ".to_string(),
]),
trigger_characters: Some(vec!["-".to_string(), "\"".to_string(), " ".to_string()]),
Copy link
Owner

Choose a reason for hiding this comment

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

hmm we may have a rustfmt issue here... maybe?

probably should just have one in the project

@@ -198,19 +114,15 @@ pub fn get_position_from_lsp_completion(

let tree = parser.parse(&text, None)?;
let root_node = tree.root_node();
let trigger_point = Point::new(pos.line as usize, pos.character as usize);
Copy link
Owner

Choose a reason for hiding this comment

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

is there a difference between position (line/char) and Point?

Copy link
Owner

Choose a reason for hiding this comment

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

oh i am dumb, its the treesitter struct, not our own. myb

@ThePrimeagen ThePrimeagen merged commit 0e2400d into ThePrimeagen:master Sep 15, 2023
1 check passed
@ThePrimeagen
Copy link
Owner

this does appear to be a comprehensive change that is a good step forward

thank you :)

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.

3 participants