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

Consistently Indent By 4 Spaces #617

Closed
Kurt-von-Laven opened this issue Feb 19, 2022 · 8 comments · Fixed by #623
Closed

Consistently Indent By 4 Spaces #617

Kurt-von-Laven opened this issue Feb 19, 2022 · 8 comments · Fixed by #623
Milestone

Comments

@Kurt-von-Laven
Copy link
Contributor

Kurt-von-Laven commented Feb 19, 2022

#346 doesn't work great with EditorConfig. For C#, one typically sets indent_size = 4. When using editorconfig-checker however, one has to set indent_size = 2 to avoid throwing errors on nested ternary statements, for example. That then causes the editor to tab 2 spaces rather than 4, largely defeating the purpose of specifying an indent_size in the first place. It seems simpler to expect folks who want less indentation not to put an inordinate amount of code in a single statement than to use 4-space indentation 99% of the time and 2-space indentation 1% of the time. I'm positive there are other tooling scenarios that are affected by the 1%.

@belav
Copy link
Owner

belav commented Feb 21, 2022

Getting csharpier to work with EditorConfig formatting is not currently a goal. If it required just a couple of changes, I could see doing it, but I did a test with just a basic EditorConfig like this

[*]
charset = utf-8
indent_style = space
indent_size = 4
insert_final_newline = true
trim_trailing_whitespace = true

Which resulted in all of these changes in the current set of test files
https://github.com/belav/csharpier/pull/620/files

After looking at editorconfig-checker, it sounds like it only supports checking basic formatting, which could be accomplished with dotnet csharpier . --check

If you are using an editorconfig to enforce non-formatting rules, you can use dotnet format to apply the fixes and validate nothing else needs to change see https://github.com/belav/csharpier/blob/master/Docs/IntegratingWithLinters.md#dotnet-format with the --verify-no-changes option

@Kurt-von-Laven
Copy link
Contributor Author

Kurt-von-Laven commented Feb 21, 2022

I think we might be discussing subtly different things. EditorConfig itself is not an auto-formatting tool, so the non-trivial diff you linked must have been generated by something else (possibly Visual Studio 2022's on-save automated code cleanups?). I totally agree that compatibility with whatever tool generated those changes shouldn't be a goal of CSharpier. I'm only talking about compatibility with editorconfig-checker, which doesn't automatically format anything, and is a far less ambitious goal. This one issue is the only incompatibility between the two in our entire code base.

I do think that regardless of all of this, mixing indentation sizes detracts slightly from the developer experience, because it makes it harder to write code in the style of CSharpier. Not only is the mixing a difficult rule to remember, but it's also not likely to be an available editor setting without the creation of many CSharpier editor plugins. The result is code that is harder to read until the next time I run CSharpier, by which point many developers won't be going back to read it anyways (not that I advocate that, ha ha).

Disabling EditorConfig (for C# only, but continuing to use it for other languages) and using dotnet csharpier . --check probably makes a lot of sense for some projects. In our case, we are already running CSharpier via pre-commit, so there is no need to also run its checker, but it's nice to know that it exists. The other drawback is that then the editor isn't advised as to the indentation style.

Having tried ReSharper's CleanupCode, dotnet format, and CSharpier, I must say I find the latter overall the best experience, so thank you for creating it! Not having to remember to hit tab twice 99% of the time is the main thing I miss about the others. Since we are using pre-commit, dotnet format's slower performance is a major deterrent to using it beyond running it ad-hoc from time-to-time, but it's quite helpful to know how to do that without breaking anything from CSharpier's point-of-view, so thank you again!

@belav
Copy link
Owner

belav commented Feb 22, 2022

I think we might be discussing subtly different things.

Yup, I realize my mistake now. I am ashamed to admit I haven't used an editorconfig before. I went from stylecop to prettier to building csharpier. I made the assumption that I needed to format all of the files (with rider + an editorconfig in this case), then compare them to what csharpier output. Really I just needed to run editorconfig-checker on files that csharpier has already formatted and see what it complains about. I can check that out and see what it will take to get them aligned. The mixed indentation came from prettier, but I don't think there is a strong reason to keep it.

it makes it harder to write code in the style of CSharpier.

I started using csharpier format on save a while ago, and I can't go back now. I spend almost no time on formatting code beyond line breaks and just save to let CSharpier fix everything for me. There are plugins now for rider, VS and vscode. And a file watcher can work for any other editors, although it is a bit slower.

Since we are using pre-commit, dotnet format's slower performance is a major deterrent to using it

If you use Husky.net it's possible to run dotnet-format just on the files that were changed. See this example. Unless you are already doing that and dotnet-format is still slow...

I must say I find the latter overall the best experience, so thank you for creating it!

You are very welcome!

@Kurt-von-Laven
Copy link
Contributor Author

Yup, I realize my mistake now. I am ashamed to admit I haven't used an editorconfig before. I went from stylecop to prettier to building csharpier. I made the assumption that I needed to format all of the files (with rider + an editorconfig in this case), then compare them to what csharpier output. Really I just needed to run editorconfig-checker on files that csharpier has already formatted and see what it complains about. I can check that out and see what it will take to get them aligned. The mixed indentation came from prettier, but I don't think there is a strong reason to keep it.

Yep, that all sounds right to me. Prettier is such a great tool. They list their community plugins on their website. I hope they would be open to mentioning CSharpier in some capacity. Whenever you feel like it's a good time for your project to attract some publicity, I'm happy to open a pull request (possibly to this file?) to add CSharpier to their list of mentioned projects.

We wound up adding EditorConfig initially to help deal with variation in editors and settings and eventually also to get rid of those pesky UTF-8 BOMs Visual Studio so loves since we are working cross-platform. Every now and then I discover some new use of it though.

I started using csharpier format on save a while ago, and I can't go back now. I spend almost no time on formatting code beyond line breaks and just save to let CSharpier fix everything for me. There are plugins now for rider, VS and vscode. And a file watcher can work for any other editors, although it is a bit slower.

Woah, that is super impressive for such a young project. Sharing the good news with my team and installing some plugins now. Good point about a file watcher too.

If you use Husky.net it's possible to run dotnet-format just on the files that were changed. See this example. Unless you are already doing that and dotnet-format is still slow...

More woah. Thanks for the tip; we wound up going with pre-commit after trying Husky, but I completely missed Husky.net. I just double-checked, and MegaLinter (which we also run via pre-commit) already runs dotnet-format on only the files that were changed. Part of the problem is that we are making a lot of sweeping changes to our code base, so that optimization only gets us so far.

@belav
Copy link
Owner

belav commented Feb 26, 2022

I'm happy to open a pull request (possibly to this file?) to add CSharpier to their list of mentioned projects.

That would be awesome. I think it is ready to start trying to attract some more attention. I actually added csharpier to https://prettier.io/docs/en/related-projects.html a couple months ago, but having it on their homepage would be nice.

I have a more realistic look now at what would need to change to get csharpier to conform to indent_size=4 at https://github.com/belav/csharpier/pull/623/files

Besides conditionals there are also SwitchExpressions, ConstraintClauses, and BaseTypes

@Kurt-von-Laven
Copy link
Contributor Author

Apologies for the delay; just got back from a trip.

That would be awesome. I think it is ready to start trying to attract some more attention. I actually added csharpier to https://prettier.io/docs/en/related-projects.html a couple months ago, but having it on their homepage would be nice.

I've opened prettier/prettier#12428; fingers crossed!

I have a more realistic look now at what would need to change to get csharpier to conform to indent_size=4 at https://github.com/belav/csharpier/pull/623/files

Besides conditionals there are also SwitchExpressions, ConstraintClauses, and BaseTypes

That makes sense. My teammate accidentally discovered another benefit of making this change at this point. As I understand it, the CSharpier VSCode extension works best when editor.formatOnSave is enabled, but that leads to conflicts with the EditorConfig VSCode extension. The latter will fight to indent all C# code by 2 spaces. One option would be to specify indent_size = unset in your .editorconfig, but this prevents EditorConfig from doing its job of configuring a huge number of possible editors correctly in one central location. Another option would be to disable the EditorConfig VSCode extension when working with C# and re-enable it when working with other languages, but that is quite error-prone. The EditorConfig VSCode extension has nearly 5 million downloads, and I would hazard a guess that a disproportionately high percentage of the engineers who would adopt CSharpier are already using EditorConfig or similar tools.

@ddaspit
Copy link

ddaspit commented Mar 16, 2022

As a side note, it would probably be useful to add support for CSharpier to obtain some configuration from an .editorconfig file if it exists. Prettier has this feature. It will be useful for projects that would like to adopt CSharpier and are already using EditorConfig.

@Kurt-von-Laven
Copy link
Contributor Author

That probably deserves its own issue. I think a minimum viable support of EditorConfig would be to use max_line_length since that is intentionally the only configuration option currently offered by CSharpier as I understand it. It should be noted that the hard line wrapping definition in the EditorConfig documentation doesn't quite line up with a CSharpier (or Prettier) style approximate line length target, which would produce another incompatibility with editorconfig-checker. I don't know how much work supporting different settings in different directories would be, but I could imagine that being a nightmare if CSharpier doesn't already have support for that. I can certainly see why Prettier has an option to disable parsing EditorConfig, but it's also a nice feature if you aren't using editorconfig-checker or are disabling it for C#.

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 a pull request may close this issue.

3 participants