-
Notifications
You must be signed in to change notification settings - Fork 503
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
Is it possible to enforce parameters are on their own line? #620
Comments
Should be linted by "parameter-list-wrapping" rule. Could you add a test case for your problem and check if it covered by this rule? |
I looked this up a little bit, and looks like this is actually a never-implemented argument-list-wrapping. The PR has been closed in favour of IndentationRule - so I guess this use-case should go there. Or we might need to think of introducing that rule nonetheless (or perhaps extending parameter-list-wrapping to support arguments). |
Sorry I haven't had much time to come back to this issue. I'll dig in this week and try to add a test case like Tapchicoma suggested and report back on that! |
Yeah, it looks like this would be called for function parameters but not for call arguments:
I will dig into how it's linted on the function parameters & see if I can reuse that logic at the call sites, too. Let me know what you think about extending |
Hi! Any updates on this? |
Wow I forgot to follow up on this! Assuming it hasn't been fixed under a different ticket, I can revisit this weekend. |
I'd say extending parameter-list-wrapping is fine in this case, as they are very similar to each other, and I can't imagine using one w/o another. But in case there's not much in common for implementation of these rules, probably a new one can be created |
Very close to a working solution: https://github.com/pinterest/ktlint/compare/master...AdamMc331:KTL-620/argument_wrapping?expand=1 However, I broke another test inside
With my change, this tries to format the arguments inside the annotation, and I have a feeling we don't actually want that based on the name of the test. So, I wonder if there's some way to see if it's |
@AdamMc331 yeah, it is
Have you seen the PsiViewer IntelliJ plugin? Works best for me in those cases |
I've just checked it - idea itself reformats it this way for me with official styleguide applied:
So I'd say it's fine to introduce this change. |
Sounds good, will update test & get a PR up soon. |
I don't get the exact same format in the unit test, I get something like this:
That's why I was worried to add this. |
It wants to put the brackets in a weird space and idk why. You mentioned "with official styleguide applied" is it possible that's not the default in a unit test? |
What I meant is when you put this code into idea and hit the autoformat hotkey it will format to what I mentioned above. But this seems like we need to maintain indentation here as well.. Just curious, if you try to add something more sophisticated as a test case, like this:
How would it reformat? |
So that one is able to print what I'd expect:
I've added the test case: 1c40419 Let me look into the linting rule a little more for the indentation and see what I can find. I'm guessing that square brackets and parens are different PSI elements so they're treated differently. Might have to dig into the plugin you mentioned. 🕵️ |
yeah, that's good, maybe the rule just needs additional handling for the square brackets |
Yeah it looks like it's treating So if I can tweak the rule to look for an argument that starts and ends with a |
I got closer to a working solution by considering elements inside a collection literal: 95bf698 Unfortunately it's not perfect, as something about the whitespace before the right bracket is screwy. But we're getting closer haha. |
Why was this changed? The official coding style allows multiple parameters on the same line for method calls:
|
@alexm-lasso yeah, we realized that we should've put this into the experimental ruleset first to make it opt-in rather than opt-out and should've not merged it with parameter-list-wrapping.. we'll try to make this happen for the next release |
I've found that this change is causing an issue where enforcing the parameter list wrapping can then introduce indentation errors, leading to an issue that is not fixable by running ktlint itself. The issue I'm experiencing is that when an For example .subscribe({
if (foo.isBlank() && !Util.isValid(foo.address)) {
bar.doThing()
} else {
bar.clear()
}
}) result: .subscribe(
{
if (foo.isBlank() && !Util.isValid(
foo.address
)
) {
bar.doThing()
} else {
bar.clear()
}
}
) manual intervention that solves the introduced indentation problem: .subscribe(
{
if (
foo.isBlank() &&
!Util.isValid(foo.address)
) {
bar.doThing()
} else {
bar.clear()
}
}
) Interestingly the Intellij formatter would break the line at edit: Aha, I found #764 which I think would fix this problem. |
I found myself in a scenario where I had multiple parameter on one line, with others on a different line, like this:
KtLint actually passes this, because there is a new line after the opening
(
and one before the closing)
, but I still think it's weird to have one parameter on one line and three on the other.I'd really prefer it be an all or nothing thing:
Any thoughts on a way to enforce this, if it's possible?
The text was updated successfully, but these errors were encountered: