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

BlueStyle: Trailing comma on single line #455

Open
omus opened this issue Aug 16, 2021 · 3 comments
Open

BlueStyle: Trailing comma on single line #455

omus opened this issue Aug 16, 2021 · 3 comments

Comments

@omus
Copy link

omus commented Aug 16, 2021

I noticed the JuliaFormatter was removing the trailing comma in this example:

@test S3Path("s3://my_bucket/prefix/that/is/fun") == S3Path(
    ("prefix", "that", "is", "fun"), "/", "s3://my_bucket", false, nothing, cfg,
)

JuliaCloud/AWSS3.jl#192 (comment)

I think the trailing comma is supposed to be included here while in this example it should not be:

@test S3Path("s3://my_bucket/prefix/that/is/fun") == 
    S3Path(("prefix", "that", "is", "fun"), "/", "s3://my_bucket", false, nothing, cfg)
@domluna
Copy link
Owner

domluna commented Aug 20, 2021

With BlueStyle when the arguments can be fit on 1 line (on the nested line), i.e.

foo(
    arg1, arg2, arg3
)

There is no trailing comma

I don't think there's a convention on keeping the trailing comma when the right parenthesis directly follows. It is also removed in DefaultStyle

cc @nickrobinson251

@nickrobinson251
Copy link
Contributor

Yeah I think the current behaviour (no trailing comma on single line) is the agreed upon one for BlueStyle

(I don't have the discussion to hand, but I think the current behaviour was something @iamed2 requested, maybe. Could have been someone else.)

The trailing comma on the single line doesn't effect the diff when another arg is added, unlike a trailing comma on arguments split over multiple lines where a trailing comma allows adding a new arg without having to change 2 lines. Also a trailer comma in the multi-line case makes rearranging args easier, which also doesn't apply to single-line case. Finally, i think it felt particularly unnecessary when adding a trailing comma to the single-line case takes the line past the char limit and causes the args to need to then be split over multiple lines. (I think these were the reasons stated for the current policy)

Also in such a minor csse, I think BlueStyle being the same as DefaultStyle is quite nice (but that's less important than the point above I this)

@omus
Copy link
Author

omus commented Aug 30, 2021

I'll open an issue in the BlueStyle repo. It's not explicitly called out but there are a few examples in the style of:

arr = [
    1, 2, 3,
]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants