-
Notifications
You must be signed in to change notification settings - Fork 746
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
Formmatting and folding code issues #13676
Comments
It seems also issue 1 applies for functions like map(). |
Issue 1 stems from a deliberate choice made by the team, but I agree that wrapping if expressions can lead to indentation that is not aesthetically pleasing. Additionally, it appears that most customers prefer a longer width setting, diminishing the rationale for maintaining such wrapping. I'm going to modify the layout rule to exempt if expressions from being wrapped. |
Would you mind sharing an example for map()? I don't there is a special case added for it. |
@shenglol This issue started a few weeks ago for me as well, the bicep module ignores the formatting rules set in VS Codes and enforces a specific bicep code formatting... A format that I don't agree with at all. In addition to @slavizh examples this also true on other circumstances as well like e.g.:
Which will be formatted as
A format I don't fancy at all. Please revert the change, or at least configure it so that it honors VS Code formatting options. It would be nice that if you want to do something like this that you implement I have inactivated |
map() example
|
Basically we need strict formatting. Only when width is reached than format on new line. And if you have multiple statements do not put them all in new line just make them in a way that the new line honor the width. |
I also don't like that formatter always moves arguments into new lines. Sometimes I by purpose split or join arguments into one line to add some comments. the new formatter breaks it all. it also treats a comment as an argument and moves it to a new line also. It also moves trailing closing bracket of for-loops into 2 lines: }] now: }
] I'd like to keep it compact. At some point we decided that we allow objects and arrays to be defined in one line to not stretch bicep templates vertically. Now the formatter brings the problem back - templates are again unnecessarily expanding. In my opinion, the new formatter is too aggressive. |
The formatter respects the newline before the first array / object item. If you don't wrap the item then it will format it as single-line array / object, unless it exceeds the width limit. If long single-line array / object is preferred, you may simply increase the width value in bicepconfig.json.
Can you share an example? I think it only does so if joining them into one line results in parsing error. For instance, it is illegal to flatten var myArray = [
true // comment
false ] into var myArray = [true // comment, false ]
This decision is intentional to ensure uniform indentation levels, aligning with scenarios where an array of objects isn't produced by a for-loop expression.
I understand that some might think the formatter is too strict in some cases. But it's tough to make a tool that everyone agrees with because people have different tastes. A formatter that is somewhat opinionated could be more useful, especially in team projects, because it makes things predictable and consistent and stops people from nagging about how the code looks during code review. |
Just to clarify, there is a |
Makes sense. The warpping of the
will be formatted as
to improve readability, even if it doesn't exceed the width limit. However, the map function case is interesting because backendHttpSettingsCollection: map(foo.array, array => {
name: array.name
id: array.id
}) is more compact and does look cleaner than backendHttpSettingsCollection: map(
foo.array,
array => {
name: array.name
id: array.id
) I'll add a heuristic to avoid breaking function argument list if the last element is an object. |
Ok, I'll take a look at the |
@mumian Do you mind helping add documentation for the new formatting settings in bicepconfig.json? {
"formatting": {
"insertFinalNewline": boolean, // Default value is true
"newlineKind": "LF" | "CRLF" | "CR", // Default value is "LF"
"indentKind": "Space" | "Tab", // Default value is "Space"
"indentSize": integer, // Applicable when "indentKind" is set to "Space". Default value is 2.
"width": integer, // The maximum number of characters that should appear on a line. Default is 120. The formatter will try to wrap the line if the width limit is reached.
}
} |
I REALLY can't get this to work. First I tried to create a Next, I tried to create a file from Visual Studio Code according to this documentation, but that doesn't create any file and just goes in a loop where I should specify the folder. EDIT: This is the section I test on
And it is formatted like
|
The if expression is something that we are going to fix for the next release (please see my response above here: #13676 (comment)). The width setting would work for the |
Ok, I misunderstood then.. This is an overall problem that it messes with the code then and until it's fixed we'll have to disable format on save then so it doesn't mess with our code 👎 And by the way, I think you really need to revisit the whole formatting, not only on |
I'd like to assure you that this was indeed what we've done. JavaScript / TypeScript -> Prettier, Biome The formatters list above all implemented the same or similar pretty-printing algorithm to enforce opinionated formatting rules with length / width limits. The offical C# formatter ( I understand having strong opinions about the "right" code style, but I think the general consensus of many people is that it doesn't matter what the specifics are, as long as they're being applied consistently. As pointed out by Prettier's Option Philosophy, the idea of having consistent formatting rules is to stop all the ongoing debates over styles like this. That being said, one potential solution to accommodate varying preferences could be implementing a feature like Discussing the .editorconfig, it seems there might be some confusion about how it works. Essentially, |
I will not continue to try reasoning to have a more flexible solution now it isn't worth the time. If you just fix so we can use width in all scenarios to avoid having the formatter to automatic line breaks so I can get back the old functionality and I will be ok with that. |
Updated two formatting rules based on [user feedback](#13676). Main changes: - The formatter no longer put `if` expression on a different line unless there is a single-line comment between `=` and `if`. - If the last function argument ends with a multi-line object or array, the function argument list will not be forced to wrap, unless the width limit is exceeded. This happens to be a heuristic implemented by Prettier. Closes #13676. ###### Microsoft Reviewers: [Open in CodeFlow](https://microsoft.github.io/open-pr/?codeflow=https://github.com/Azure/bicep/pull/13945)
@shenglol Is there a plan to change the formatting of |
Bicep version
Bicep CLI version 0.26.54 (5e20b29)
Describe the bug
There are two bugs/issues that I see with latest version. Most likely applicable for previous versions as well.
Issue 1
even that I have following formatting configuration:
The width is not honored by if statements on resources. Those are always moved on the next line no matter what is the width defined. For me this is an issue as when possible I want to use the maximum width available and only multiline when that width threshold is reached. It should apply to any part of the code so if if statements do not exceed that threshold they should not be moved to new line.
Issue 2
When you have if statement on the resource and that if statement is moved to next line the whole indent is moved one space further. That causes the last closing bracket '}' to be indented and when you fold code it folds it to the next resource instead of folding it to the last bracket.
To Reproduce
test.bicep
Issue 1:
After document format:
Issue 2:
Additional context
Add any other context about the problem here.
The text was updated successfully, but these errors were encountered: