Reformat code base

[pr8x]

Are there any plans to reformat the entire code base at some point? There are quite few spots with inconsistent indentation or order of properties/fields/functions.

[robloo]

In terms of controls I would expand this topic even further: Refactor Avalonia.Controls #4957

[robloo]

Even doing it one at a time would be a help. All the Grid related classes should be cleaned up to start. Then a new Buttons directory will be needed soon.

[robloo]

The current inconsistency in directory also bothers me :) ItemsRepeater for example you decided to put in one directory. That's done for a lot of the newer control additions. Flyout, etc. However the original controls are put all over the place. In huge projects it starts to make sense to organize by feature. Even MVVM monolithic ViewModel folders usually turns into ViewModels being placed in a directly along with the View itself. Its just a better way to organize large projects: by feature, IMO.

[grokys]

The problem with putting everything in directories is that VS uses that directory structure for the namespace when creating new files, meaning that people start committing files with incorrect namespaces. I've done it a few times myself but thankfully noticed before submitting a PR. We've got a few examples of that happening still in the codebase and we can't fix the namespaces until the next major release because it's a breaking change.

Because of this, directories that don't match the namespace of the classes within should be avoided unless really necessary (i.e. when a control has a lot of files in it (i.e. ported from WinUI)). Even then, I'm not sure it's the correct choice.

I'd consider changing my position on this only when VS gives us some way to not create broken APIs by default (I think Rider has the same behavior too).

[maxkatz6]

Rider handles it better. If files in the folder do not have new deeper namespace, new file won't have it either. I would expect Resharper to do the same.

But yes, it's annoying with VisualStudio.

[robloo]

You're the boss :) I would say that sounds like a bit of a reach. True, namespace defaults can catch you sometimes but its standard practice to modify these and also to review them in commits. I dont know of any project that follows namespace directory structure so precisely that no modification is ever necessary. The irony here is Avalonia already has this issue so it wouldn't be a new problem at all.

[maxkatz6]

Merging of some assemblies is definitely planned
#5831

[grokys]

Personally I'd love to! I often get frustrated about the inconsistencies. However there are 2 problems:

1. Most common pushback on such ideas is that: it introduces noise to git blame, makes cherry-picking between master and release branches more difficult, and causes extra work for people with open PRs
2. If we don't have an automated tool to fix such errors before committing, we'll be back here in 2 years

It's a difficult balance between the camp who wants consistency and the camp who feels that consistency is a distraction and just want to get on with solving problems. I'm in camp A, btw.

[pr8x]

I think I said that before, but: Sacrificing readability, consistency and by that maintainability of a code base because it introduces minor inconveniences to some git tools is really stupid. We could have a one month notice period to flush pending/obsolete PRs (that should be done frequently anyway), so they won't conflict too much.

git blame actually allows to ignore a certain commit via --ignore-rev (see https://akrabat.com/ignoring-revisions-with-git-blame/). We can even ignore an entire list of commits via --ignore-revs-file so whenever reformat patches are done we add the commit hash in there.

[robloo]

Fully agree with @pr8x and have also said elsewhere git blame is a poor reason to reject changes like this.

However, I also agree that automated tooling is needed to enforce this going forward.

[maxkatz6]

In some cases it makes "git blame" completely broken without any chance to fix it (yes, I am looking at you, GitHub, who does not follow moved files in blame view).

[maxkatz6]

Although I still agree, that it makes sense anyway, especially when we already planning to break git history with #5831

[robloo]

Yea, support for --follow in GitHub is a sore subject. They've been asked to add it almost since the beginning and there is no movement. I'm guessing it requires a lot more processing power or something and they just don't want their server costs to go up. That said, they don't need to run it by default and it could be an additional button in history like an old extension someone wrote for chrome.

[grokys]

Ok, so if we were to fix up formatting as part of #5831 does anyone have suggestions for preventing the codebase getting into a mess again?

The options I'm aware of are Rider/Resharper and StyleCop. Anything else? Something like gofmt for C#? Anyone used https://github.com/dotnet/format?

[kekekeks]

However, it could be done for each PR after the initial code

That would force us to squash PRs. We avoid doing that when possible.

[robloo]

A minor trade-off it seems to cleanup the code base. Many other repos automatically squash too. That has some benefits itself although you might disagree.

I've noticed the quality of code formatting with several recent commits in inconsistent and even suboptimal. The code base is going to get messy unless core maintainers become much more strict in code review or an automated tool is used. Everyone appreciates a clean code base that is easy to navigate and consistent.

[robloo]

I suppose there is a best-of-both worlds solution. Have a tool that analyzes PRs for code formatting errors and simply block merging until they are fixed. It would be a form of code analysis but basically raises errors on formatting differences. This would force committers to fix their formatting to follow the rules -- but it wouldn't be done automatically.

No tool exists to do this I'm aware of though tools like Codacy can do some of it.

[kekekeks]

Without reformat-on-build we are pretty much guaranteed to have "Fixed formatting" commits in PRs. I don't think there is an existing OSS solution for doing this, so I suppose that we need our own MSBuild task and either scrap CodeFormatter for parts or just use Roslyn directly since Microsoft.CodeAnalysis.CSharp.Workspaces has a decent code formatter with configurable rule set.

We also need to decide if we want just code formatting or enforcement of file/class layout. Personally I don't like that because it doesn't allow to group logically interdependent parts like avalonia property declarations. There is also a problem with auto-fixing such things. We could use roslyn, but it might break stuff like conditional compilation. Without auto-fixing such rule will be way too annoying.

[robloo]

MSBuild task and either scrap CodeFormatter for parts or just use Roslyn directly since Microsoft.CodeAnalysis.CSharp.Workspaces

Yes, a Roslyn-based solution is likely the best open-source option. Though JetBrains internal tooling is probably still more powerful and general purpose. Microsoft is struggling these days (less so after .NET Core) and isn't at the level of skill they used to be. At the turn of the century they had the best engineers and great management -- not so these days. All this is to say, like Avalonia itself, the less you rely on Microsoft the better.... I digress

enforcement of file/class layout.

Personally, I would require this. Rules like: fields at the top of the file, organized members by public/private, methods all together, etc. I still like the idea of putting Properties in their own <Control>.Properties.cs file as WinUI is doing these days. It separates them out from logic and just makes things easier to read -- and alphabetically sorted. I know you guys don't want to do this yet though.

Finally, I think this should be opt-out with a comment analogous to // <auto-generated /> which disables code analysis. The example you gave where some things make more sense to arrange by logically interdependent parts is a good special case. Also source code pulled from WPF/WinUI like Grid should not be changed to avoid loosing the ability to copy over updates. This would mitigate some of your other concerns as well.