Closed alexpw closed 1 year ago
Really great write up, and really great effort. I'll think this over, then get back to you with a plan for getting this merged :)
Alright! Since there's a lot more complexity in here than I had to do for ruby, I figured it was a good opportunity to build the test framework. #21 implements it, and I'll be working on that this weekend when I have time. Do you also mind taking a look at the new helper I added, destructure_node()
? I think it might be handy for teasing apart a tree into it's parts without needing to rely on the node's positions.
... build the test framework. #21 implements it, and I'll be working on that this weekend when I have time.
Cool! It's looking great so far and I'm eager to use it, as soon as you think it's ready. I have so many (manual) test cases, ugh, lol...
... the new helper I added,
destructure_node()
? I think it might be handy for teasing apart a tree into it's parts without needing to rely on the node's positions.
I thought so too! I was excited when I saw it, hoping I could get python.lua to resemble how clean ruby.lua is, but after a quick try, I didn't see it produce usable/equivalent structures for the node or parent. But I'll try again with child nodes and/or rethink.
Fundamentally, I'm trying to make it so when using it, you're not distracted by thinking whether it makes sense syntactically to inline or expand or whether the plugin supports this case or whether it acted properly. Therefore, the plugin has to support (nearly) all valid situations, but only when it can safely do so. But obviously adds complexity to the implementation that's been difficult to abstract.
Speaking of that, I realized I could safely support inlining multiline parenthesized_expression
's and allow boolean_operator
, and I'll finish testing that shortly, then do more clean it up tomorrow. 😅
In general though, when inlining, I replaced all calls to helper.node_text()
with a wrapper that calls collapse_chlld_nodes()
if it's multiline.
if (True and
False):
return [3,
4,
5]
else:
return (4 or
6)
Inlined as
return [3, 4, 5] if (True and False) else (4 or 6)
Speaking of complexity, if instead of return (4 or 6)
above, it was this:
return 4 or 6
When inlining, it sees a bare boolean_operator
and wraps it in parens to remove ambiguity.
I merged the testing framework into master - you should be able to rebase and start using it. There are a handful of ruby examples, but since it's new just lemme know if anything about it is unclear or needs to be added :)
toggle_multiline.lua:collapse_child_nodes()
now ignores/skips embedded comment
nodes, where previously it produced invalid syntax.Hopefully this looks good to go to you, or close to it.
As a teaser, because maybe it'll spark ideas for you elsewhere, I have another branch to submit soon that expands {list,set,dict}_ comprehension
s. eg:
xs = {x for x in range(10) if x > 3}
becomes
xs = set()
for x in range(10):
if x > 3:
xs.add(x)
Really great work with all of this! If you feel like it's in a good spot then I'll play around with it a bit and see about getting it merged (time permitting) over the next few days :)
Had a chance to read this over - good lord, Python is fussy. Bravo. My only "major" note is that I'd rather not strip comments out. If we encounter a comment, I think it's more reasonable to bail than to throw the comment away.
Really good stuff. Let's get this merged and we can iron out any issues in a future PR. Thanks again for the contribution and effort :D
Hi!
This plugin has been super helpful for me (thank you!), so I built out more python support. I tried to keep commits isolated to
filetypes/python.lua
to avoid design decisions, but there are a few non-breaking changes outside of it. I try to give context/explain the reason why for each (see below: Core Changes).Features:
Comprehensions:
The result of toggle_multiline() on the complex example (cursor on the square bracket):
Inlined if statements:
are expanded (via _nodeaction on the
if
) to:These expanded forms can all be inlined, but when it can't safely do so or when it doesn't make sense to, it won't.
Core Changes
init.lua:do_action()
- An optionaltarget_node
can be returned by actions and used as the target for replacement.This was used for python's "if/else <-> ternary", where the parent of the node being acted on is instead the one that needs to be replaced. For example:
The
if
is a "conditional_expression" that initiates the node_action. Once expanded, theif
is again the target for the inline_if_stmt node_action.init.lua:replace_node()
- The options returned by an action gained atrim_whitespace
key that trims trailing whitespace between thestart_row
andend_row
. It is a table expecting keys by the same name, with relative values adjust the default ones, to match the cursor row/col design and behavior.This was necessary to support expanding:
The expanded replacement needs to go on the next line and leaves behind 1 trailing whitespace. For example, expanding this becomes:
The
_
marks the whitespace error, which is a gap left between thefor_statement
and previously inlinedbody
. The only way around this that I saw, was to leverage (1.) to replace the entire for/if statement node, but maybe the whitespace trimming will be useful to others.helpers.padded_node_text()
can now acceptpadding
with values that are optionally a table instead of a string. I did my best to document it with comments, so I won't repeat it here, since this is already a wall a text. It receives aprev_text
param fromtoggle_multiline.lua:collapse_child_nodes()
that allows it to be semi-context aware.The tldr reason why, is that the expected
_is_not_
and_not_in_
would be padded as_is__not_
and_not__in_
and it needed to be smart enough to only have 1 space between them. But also for unary/binary-
, it can now pad differently for each.