Skip to content

Conversation

@CKolkey
Copy link
Owner

@CKolkey CKolkey commented Feb 26, 2023

No description provided.

@alexpw
Copy link
Contributor

alexpw commented Feb 26, 2023

Thank you for refactoring the padding and this approach, being self-contained, is definitely cleaner.

Also thank you for the style and doc comment cleanup. Do you use a style guide or equivalent of stylua? I tried to match the existing style as best I could, but it's a subjective guess sometimes.

For helpers.node_text(), in #25, I elected for a separate version (node_multiline_text()) that always return a table, for consistency. I found that if a node can't be multiline, I safely call node_text() for a string and otherwise, call the multiline variant and use the table. I found that helped write cleaner code. Just something to consider. I'm not sure of the best approach.

@CKolkey
Copy link
Owner Author

CKolkey commented Feb 27, 2023

Yea, no worries man :) It took me a bit of playing with to figure out the context thing, but in the end I think you solved the problem really nicely.

Truth be told, the style stuff is mostly just whatever the lua-lsp autocorrects to. But I don't mind little things here-and-there, so no worries. It is pretty subjective at the end of the day.

I'm leaning towards changing the function signature for the helper to always return a table, like in your version, even for single line nodes. But I'm not sure. It would be consistent with how an action can return a string for single line things, and a table of strings for multi line. I think I'll put off making that decision for now.

@CKolkey CKolkey merged commit 14febd0 into master Mar 7, 2023
@CKolkey CKolkey deleted the change-node_text-behaviour branch March 7, 2023 20:12
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