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

Remove v1 formatter #13290

Merged
merged 18 commits into from
Feb 20, 2024
Merged

Remove v1 formatter #13290

merged 18 commits into from
Feb 20, 2024

Conversation

shenglol
Copy link
Contributor

@shenglol shenglol commented Feb 10, 2024

The PR removes the v1 formatter from the codecase.

Will send another PR to remove the prettyPrinting feature flag given that the PR is already overloaded with many file changes.

Closes #11733.

Microsoft Reviewers: Open in CodeFlow

Copy link
Contributor

github-actions bot commented Feb 10, 2024

Test this change out locally with the following install scripts (Action run 7978058887)

VSCode
  • Mac/Linux
    bash <(curl -Ls https://aka.ms/bicep/nightly-vsix.sh) --run-id 7978058887
  • Windows
    iex "& { $(irm https://aka.ms/bicep/nightly-vsix.ps1) } -RunId 7978058887"
Azure CLI
  • Mac/Linux
    bash <(curl -Ls https://aka.ms/bicep/nightly-cli.sh) --run-id 7978058887
  • Windows
    iex "& { $(irm https://aka.ms/bicep/nightly-cli.ps1) } -RunId 7978058887"

Copy link
Contributor

github-actions bot commented Feb 10, 2024

Test Results

    66 files   -     33      66 suites   - 33   21m 51s ⏱️ - 28m 17s
10 641 tests  -    395  10 640 ✅  -    396  1 💤 +1  0 ❌ ±0 
25 172 runs   - 13 707  25 170 ✅  - 13 709  2 💤 +2  0 ❌ ±0 

Results for commit c1f1fdc. ± Comparison against base commit 4e07d82.

♻️ This comment has been updated with latest results.

@shenglol
Copy link
Contributor Author

Should probably merge this after the hotfix release is published.

@shenglol shenglol added do not merge Do not merge this pull request yet. and removed do not merge Do not merge this pull request yet. labels Feb 12, 2024
options = options with { IndentSize = args.IndentSize.Value };
}

if (args.Width is not null)
Copy link
Member

Choose a reason for hiding this comment

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

Did you make a decision on what the default width should be?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not yet, but I'm leaning towards 100 as an optimal value - 80 feels a bit constrained, while 120 might be too wide for those without high-resolution screens. I'll arrange for a vote during our upcoming Bicep community call to decide and will update the code accordingly.


if (args.Width is not null)
{
options = options with { Width = args.Width.Value };
Copy link
Member

@anthony-c-martin anthony-c-martin Feb 13, 2024

Choose a reason for hiding this comment

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

I'm slightly concerned that people may be opinionated about making the decision to automatically wrap vs preserving formatting.

For example, if I've chosen to write:

var foo = [
           'short'
]

I might expect the formatter to format it to:

var foo = [
  'short'
]

But this feels unexpected and possibly undesirable:

var foo = [ 'short' ]

Is there any ability to configure this behavior?

I'm probably more opinionated than most, but to give an example, I personally strongly dislike the lack of freedom over formatting that we have in our .ts files in the src/vscode-bicep project. I find a lot of the automatic formatting far less readable, and often unneccessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, no, but this is implemented for objects, and we can easily turn it on for arrays. Would you consider this a valuable topic to bring up for discussion at our upcoming Bicep community call?

Copy link
Member

Choose a reason for hiding this comment

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

By the way, my point here isn't to say "my way is better", I'm just using myself as an example of "people can be have strong opinions about formatting".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this may make more sense if we end up deciding to use a larger width (say 120) as it may lead to long single line arrays 🤔.

Copy link
Member

Choose a reason for hiding this comment

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

Currently, no, but this is implemented for objects, and we can easily turn it on for arrays. Would you consider this a valuable topic to bring up for discussion at our upcoming Bicep community call?

Yeah, I think it definitely wouldn't hurt to bring up. I'm not sure what sort of usage the current formatter gets, so it's difficult to gauge how important it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could also introduce a setting to allow users to refine this behavior further. A useful feature might be the ability to split objects and arrays not just based on width, but also considering the quantity of properties or items they contain (e.g., always break an array if it contains more than 2 items).

Copy link
Contributor

@StephenWeatherford StephenWeatherford left a comment

Choose a reason for hiding this comment

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

some comments

src/Bicep.Cli/Arguments/FormatArguments.cs Outdated Show resolved Hide resolved
src/Bicep.Cli/Arguments/FormatArguments.cs Show resolved Hide resolved
src/Bicep.Cli/Arguments/FormatArguments.cs Outdated Show resolved Hide resolved
src/Bicep.Cli/Arguments/FormatArguments.cs Show resolved Hide resolved
src/Bicep.Cli/Arguments/FormatArguments.cs Outdated Show resolved Hide resolved
src/Bicep.Core/FileSystem/PathHelper.cs Show resolved Hide resolved
@shenglol
Copy link
Contributor Author

Had an offline chat with @anthony-c-martin, and we agreed to reintroduce the v1 formatter after this PR is merged. We'll implement a new feature flag enabling users to revert to the v1 formatter in the Bicep CLI and Bicep VS Code extension, in case there are any overlooked regressions with the v2 formatter. This approach ensures a more secure migration strategy.

@shenglol shenglol merged commit bce983b into main Feb 20, 2024
44 checks passed
@shenglol shenglol deleted the shenglol/bye-v1-formatter branch February 20, 2024 18:12
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.

Deprecate v1 formatter
4 participants