Added TextObject for Tags - #194 #13 #247
Conversation
added tests + code cleanup
|
@aioutecism thoughts on this? |
|
I have never used this keypress in vim, although now I see this PR I have tried it out and it seems pretty useful! One suggestion on the tests might be to also test some "sad path" cases, specifically to show what happens when you don't have a perfectly nested tags. |
|
Thats a great idea Alison! At this point it seems like many PRs and issues
on this repo have gone stale though. I've been using this fork of the
extension for months now with no issues, so I'll continue to do that until
activity picks back up around here.
…On Sun, Oct 20, 2019 at 9:49 PM Alison Winters ***@***.***> wrote:
I have never used this keypress in vim, although now I see this PR I have
tried it out and it seems pretty useful! One suggestion on the tests might
be to also test some "sad path" cases, specifically to show what happens
when you don't have a perfectly nested tags.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#247?email_source=notifications&email_token=AAEMPPOS7W4O34GGX33CITTQPURETA5CNFSM4G4W5CVKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBY7BSQ#issuecomment-544338122>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAEMPPN3NOIWBCUDBPHMUZDQPURETANCNFSM4G4W5CVA>
.
|
|
I admit it - i had a bit of an ulterior motive in commenting - it was to see if i was the only one still here 😁 Are you just running your fork from this PR or did you merge together a few of the ones open here? |
|
:)
Just running my fork for now, but still haven't lost hope that this will
get merged! Hopefully the maintainer has just been busy the past few
months.
… |
|
@austenc sorry for the silence on this! One of the things I miss is this text object (I use I would agree that a couple of 'sad path' tests would be useful - would you like to add them and then we can merge this? @aioutecism has said they are happy to help by pushing new releases of the extension when we have things ready to go :) |
|
Added a few more tests, did you have any other specific "sad path" tests in mind @alisonatwork @jackfranklin? Thank you to both of you (and to @aioutecism !) |
|
|
||
| let match; | ||
| let tags: Tag[] = []; | ||
| while ((match = this.TAG_REGEX.exec(document.getText())) !== null) { |
There was a problem hiding this comment.
We should avoid using getText without a range. This would freeze the editor if the document is too large.
VSCode recently added a feature to mirror cursor on matching tags. Seems like it's using the language server:
https://github.com/microsoft/vscode/blob/9e1d730cf2950297d7a85fc66be89411d7b8bcd9/extensions/html-language-features/client/src/htmlMain.ts#L134-L139
This will only support HTML, but maybe that's what we want anyway?
Or we can just write a more performant matching logic, but some edge cases such as <br /> or <div devilData="<div>">got you</div> can be difficult.
There was a problem hiding this comment.
It would be good to support JSX and any similar variants if possible - not sure if using the language server would limit that?
There was a problem hiding this comment.
Yes, not to mention the other various templating languages and file extensions. Some I use often are *.blade.php or *.antlers.html for example.
I am not a huge fan of parsing the whole document, but admittedly this idea and the regex was borrowed from the other vim extension. I tried doing it other ways but ran into various issues that required having the sets of matching end tags (which my original attempts did not).
There was a problem hiding this comment.
Make sense. We can challenge this later.
@austenc Would you mind adding a comment here such as // TODO: Potential performance issue with getText()?
There was a problem hiding this comment.
Done! Thanks for the reminder 👍
|
Despite the potential performance issue and rare cases, this should work most of the time. So I'm okay if we merge it and add some comments & create an issue to remind us to fix it later. Leave it to you @jackfranklin . |
|
@austenc this is awesome, thank you!! @aioutecism I think that if we ship this + gather feedback on cases where it causes issues, that might be the best way of figuring out the next steps? |
Agree. |
|
Thank you @aioutecism for the guidance and reviews! One last thing... I am seeing tests fail sometimes. It seems to affect tests outside the scope of this PR also. Most of the time when I run them it works fine, but occasionally I'll have one fail. Haven't figured out a way to get it to fail consistently... but I'm wondering if it is a slight variability in the blackbox stuff or something? I am not too experienced with javascript testing so I don't know where to look. |
|
@austenc Thank you so much for contributing! The test failure might because VSCode sometimes is slow to respond to some commands. I haven't found a good way to avoid this. Let's wait for @jackfranklin's review and I think we can ship this then! |
|
Thanks for the contribution @austenc - I will merge this and hopefully we can release to the VS Code marketplace soon 👍 |
|
THANK YOU! 🎉 🎉 🎉 🤗 |
Hello @aioutecism and once again thank you for your guidance earlier in the week! Here's my first stab at adding
t/ tag TextObjects to the extension. Looking forward to seeing what you think! The new stuff I added uses regex to parse out open/closing tag pairs in the document, and then find the proper ones based on cursor position. In all of my tests it seems to do the job!A few things I wanted to mention which I had trouble with:
parseTagsmethod would ideally be called in a constructor, but since it needs adocumentreference it seemed best to just use a class variable to determine if we need to parse the document again or not. Hopefully this works okay?Please let me know what you think and I'll be happy to adjust so you can get this thing released.
Cheers! 🍻