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

Assignment: hang at equal token before expanding RHS #292

Closed
GlowingUmbreon opened this issue Nov 9, 2021 · 5 comments · Fixed by #342
Closed

Assignment: hang at equal token before expanding RHS #292

GlowingUmbreon opened this issue Nov 9, 2021 · 5 comments · Fixed by #342
Labels
enhancement New feature or request

Comments

@GlowingUmbreon
Copy link

Currently I have a function that gets formatted into this:

local musicId, musicTime, responseTick, responseOffset = remotes.Server.GetSpectatorInfo:InvokeServer(
	player,
	sendTick
)

In this situation I believe that this:

local musicId, musicTime, responseTick, responseOffset = 
	remotes.Server.GetSpectatorInfo:InvokeServer(player, sendTick)

Or maybe even this: (This cuts down on variables per a line which maybe is be easier to read)

local musicId, musicTime, 
	responseTick, responseOffset = remotes.Server.GetSpectatorInfo:InvokeServer(player,sendTick)

Looks better.

@JohnnyMorganz JohnnyMorganz added the enhancement New feature or request label Nov 9, 2021
@matthargett
Copy link

Current prettier output of equivalent JS, which supports the idea of breaking multiple assignment at the assignment operator (the underlying heuristic may be something else, though):

const { musicId, musicTime, responseTick, responseOffset } =
  remotes.Server.GetSpectatorInfo.InvokeServer(player, sendTick);

@GlowingUmbreon
Copy link
Author

Current prettier output of equivalent JS, which supports the idea of breaking multiple assignment at the assignment operator (the underlying heuristic may be something else, though):

const { musicId, musicTime, responseTick, responseOffset } =
  remotes.Server.GetSpectatorInfo.InvokeServer(player, sendTick);

Out of curiosity what happens if you have like 20 variables being assigned, does it do something like my 3rd example?

@JohnnyMorganz
Copy link
Owner

JohnnyMorganz commented Dec 13, 2021

Current prettier output of equivalent JS, which supports the idea of breaking multiple assignment at the assignment operator (the underlying heuristic may be something else, though):

const { musicId, musicTime, responseTick, responseOffset } =

remotes.Server.GetSpectatorInfo.InvokeServer(player, sendTick);

Out of curiosity what happens if you have like 20 variables being assigned, does it do something like my 3rd example?

I would (personally) argue that your 3rd example is more confusing than the second, since in the second it is easier to see all the variables that are being assigned on one line, then the expression(s) assigning them on the other. My intention is to implement the 2nd format (once I've got some spare time)

I imagine for an extremely large number of variables, prettier would format it to expand the "table"-like syntax for the variable destructuring, with each variable on its own line. (This is just a guess though, can't test rn, would recommend having a quick look at the prettier playground to see the actual result). This isn't something we do yet, but could be a possibility

@JohnnyMorganz JohnnyMorganz changed the title Don't newline arguments on long lines Assignment: hang at equal token before expanding RHS Jan 20, 2022
@JohnnyMorganz
Copy link
Owner

JohnnyMorganz commented Jan 20, 2022

Current prettier output of equivalent JS, which supports the idea of breaking multiple assignment at the assignment operator (the underlying heuristic may be something else, though):

const { musicId, musicTime, responseTick, responseOffset } =
  remotes.Server.GetSpectatorInfo.InvokeServer(player, sendTick);

I was looking back at testing this, and it seems that prettier's heuristics have changed, and it is now formatted differently - strange

I implemented a change in #342 which adds this in, but it seems to have some impact on the other test cases - I'm not sure which version is better in the failing tests

@matthargett
Copy link

That is interesting. When I use your prettier link above and add another parameter, it hangs at the assignment:
const { musicId, musicTime, abcdefgijklmh, responseTick, responseOffset } = remotes.Server.GetSpectatorInfo.InvokeServer(player, sendTick);

it looks like it does work “backward”, figuring out what needs to be hung based on the token group
that crosses the line length. In my above example, as soon as it’s no longer the method call parameter list that is over the line length (the cutoff line is before the first paren), it hangs at the next break. That appears to be the assignment operator, but it’s interesting that it doesn’t choose the previous . chain. It looks like there’s a definite precedence defined.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants