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

Multiline formatting of generic for syntax #337

Merged
merged 8 commits into from Jan 10, 2022
Merged

Conversation

JohnnyMorganz
Copy link
Owner

@JohnnyMorganz JohnnyMorganz commented Jan 7, 2022

Closes #322

Components.Transform,
Components.Mothership,
Components.Mothership
) do

Choose a reason for hiding this comment

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

hm. Is there a reason we don't align the do with the end, like we do for similar scope closure pairs like: function() and end, if and else?

Copy link
Owner Author

Choose a reason for hiding this comment

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

In this case, I followed how prettier did it

It seems that in a for loop, the body opening bracket is aligned with the closing parentheses, but in an if-statement they don't do this...

I think I agree, its probably better to just keep it consistent with how we do if ... then expanded multiline. Also makes it easier to handle comments

end

local function system(world)
for id,

Choose a reason for hiding this comment

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

why isn't id indented on the next line like the rest of the arguments? again, I'm thinking of consistency with the if and else cases

Copy link
Owner Author

Choose a reason for hiding this comment

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

I don't think there was a particular reason for this, just how I did it (its interesting how the the id in the first line aligns nicely with the other variables too, thats wasn't intentional).

Again, I agree its better to be consistent here so I'll change it

@JohnnyMorganz
Copy link
Owner Author

@matthargett, wanted your opinion on the changes (I can't seem to re-request your review)

@matthargett
Copy link

I didn’t test locally, but reviewing the tests it looks good to me.

@JohnnyMorganz JohnnyMorganz merged commit 7388e66 into master Jan 10, 2022
@JohnnyMorganz JohnnyMorganz deleted the for-loop-multiline branch January 10, 2022 11:28
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.

Total line length not taken into account in for loops
2 participants