Solution-wide formatting pass

[dodexahedron]

This is just to put a formal work item in for #3207.

Also, per the discussion that has happened so far in that PR, I think a dedicated branch and PR are warranted to take care of a dependency: Creating and adding appropriate dotsettings files for general use. Then, the final committed code in #3207 will be able to be the result of using and applying those rules, so that we don't end up with more changes in short order.

I've got partial definitions already, with more complete settings pending resolution of discussion around it all.

[dodexahedron]

Work on the dotsettings files tracked in https://github.com/dodexahedron/Terminal.Gui/tree/v2_resharper_style_settings_3212

[dodexahedron]

Alrighty.

What is currently pushed in that branch is almost the same as what I used to format the color PR.

The main difference is that it has inspection severities for a lot of rules turned up higher in a lot of cases. Some which are pretty basic and that have no real excuse for merging or which have real potential unintended runtime consequences are even turned up to ERROR level, meaning if you have ReSharper or Rider, it will be treated as a build failure, forcing you to fix it before continuing. Those are things like the null check pattern using == or namespaces being block scoped instead of file scoped. Those will throw errors.

Just to note what severities of inspections mean in ReSharper:

• None => Not displayed in the UI, for inspection purposes. This does not mean that the rule is disabled or not respected. That is controlled in the Formatting Style settings.
• Hint => Gets a one-character wide light-colored (theme-dependent) squiggle where relevant, but not mentioned in the code map scrollbars or in anything other than the "Find Code Issues" tool. Also does not affect the file inspection status icon at the top of the code map scroll bar, either. These really are just hints for alternatives.
• Suggestion => Gets a squiggle under the entirety of the element that it applies to, and shows up as a green tick mark in the code map scrollbar, for quick access.
• Warning - A warning. Shows up in the error list like actual compiler warnings, and shows up as a yellow tick in the code map scrollbar. Also makes the code map scroll bar file status icon change to a warning triangle, showing that there's at least one warning in the current file.

• Error =>An error. Used for things that are either problems or that are clear violations of the style rules. Treated like a compiler error, meaning you can't successfully build, test, or debug until you fix it. Shows up as a red tick mark in the code map and the status icon changes to a red error circle indicator.

  Errors are also reported in the lower-right of VS in the status bar, and you can quickly navigate through all errors, one by one, by clicking on the red text showing the count of errors and files with errors. Subsequent clicks on it advance to the next error.

I suggest, when navigating errors and fixing them, that you let things catch up before moving on to the next one. It can sometimes take a second, depending on lots of factors like project size, current document size, and your personal ReSharper and Visual Studio settings and any plugins to each.

[dodexahedron]

I have not yet added the actual profiles for auto-running Code Cleanup, which is what ReSharper's bulk auto-clean and auto-format operation is called.

On that tool, though:

When using it, the default key combo is ctrl+e,c

It will ask you what profile you want to run, when you do that, and it will indicate to you what it will apply to.

If you have a code file open and no text is selected, the default is to process the current file.

If you have nothing open, the default is to process the current project.

If you have a file open and some text selected, the default is to process ONLY the selection (super nice for some situations).

What it applies to can be overridden in most cases, if you want it to, for example, apply to an entire type (which may mean multiple files, if that type is partial, for example).

I'll put together a couple of profiles next. They're pretty simple, and just specify which categories of operations you want applied when that profile is run. They do not turn any individual rules on or off.

[dodexahedron]

Also, note that ReSharper settings are layered.

The default behavior is to apply them in this order, with the last entry that modifies a given setting being the conflict winner, BUT with any specified in a team shared file having precedence over user local settings (so people can't just override the project settings locally):

1. Team shared dotsettings file at the solution level
2. User-local solution dotsettings file (these have the .dotsettings.user extension and are already in the gitignore)
3. Team shared project-level dotsettings files
4. User-local project-level dotsettings files (also with the .user extension)

Anything not explicitly defined as different from the BASE ReSharper defaults is not included in the files, and will default first to anything you may have specified in the "Computer" layer, which is a global layer for your environment for settings you like to keep as defaults between solutions. And then, for anything that isn't defined even there, ReSharper defaults will be used, which adhere very tightly to the C# design guidelines from Microsoft.

Additional sub-layers can also be specified in additional files, if there is a desire for different settings to apply at different times.

For example, perhaps the things I put in the solution file as errors could be warnings for the base solution layer, but overridden to error in a sub-layer which is used in the CI pipeline.

[dodexahedron]

Profiles added.

Default profile is applying all rules.

Additional profiles I added are:

• A profile that only sorts the target code
• A profile that only removes redundancies, making no other changes
• A profile that re-formats only, and does not sort code

[dodexahedron]

I made a branch from the end of this branch that applies the changes necessary to address the errors that will be thrown by this DotSettings collection.

The changes consist of:

• Namespaces -> file-scoped
• Braces added where required by the rules (mostly loops)
• Constructors never expression-bodied (that's generally not recommended)
• Type keywords used instead of type names (a few Strings became strings, and IntPtrs became nints)

No other formatting or syntax rules were applied for this - just what's necessary to fix the ones I marked as error.

The two methods I mentioned in the AoT issue also are as specified in that issue already.

[dodexahedron]

Oh hey. You probably need the branch link huh?

Whoops

https://github.com/dodexahedron/Terminal.Gui/tree/v2_resharper_style_settings_fixed_build_3212

[dodexahedron]

The intention behind that branch (which is one big commit) is just to show how quick and easy it was to do it. I literally did not type a single character to make all the fixes. I just clicked on an error, told it to fix it solution-wide, and....that's it.

A note about that though:

Unless the machine you are on has a LOT of memory, don't execute solution-wide fixes like that, unless you need to go get lunch or something, because it will take a while and will take a lot of memory, since it opens each file it has to touch (it will warn you before it goes off and does that, after it determines how many files it needs to touch).

When I ran the very first solution-wide fix, VS was using over 6GB of memory, and ReSharper accounted for 4.1 of that.

In any case, that branch can be used or ignored as you wish. Just let me know, so I can delete it when appropriate.

[tig]

Did you turn on the recommended rules for var? I know I didn't fully understand the issues until you came along, and as a result I did some dum stuff that should be Error (and fixed by enforced rules).

I need your help on implementation steps. TBH, my git-fu is weak and you have a vision involving these branches that I'm not quite groking.

We have:

• [ ] v2_develop - Truth. The branch we want to be the root of future work ASAP.
• [ ] #3202 - A PR with a massive set of changes by me. I've been keeping it up to date with v2_develop.
• [ ] #3181 - A PR with an overlapping set of massive changes with #3202 by @BDisp. I've agreed that I'll do the nasty work of merging #3202 and #3181 together.
• [ ] #3207 - A PR I created to be the PR that eventually merges the code cleanup back into v2_develop
• [ ] https://github.com/dodexahedron/Terminal.Gui/tree/v2_resharper_style_settings_fixed_build_3212

How do you recommend we get this done?

[dodexahedron]

Yeah, that's where I was going with proposing we do something live, even if it's just a Q&A session. We can probably get more of your R# questions answered and maybe actual work done in less than an hour than it'll take us several days to get through, asynchronously. 🤷‍♂️

I did turn on a form of the var rules that I laid out in my suggestion above, which do what seems to be pretty darn close to what I suggested, so you could look at/play with it.

That was in commit 404b3d66c2df2c40a245e01526e971a0b56a64e2

I did that branch in a ton of tiny commits so you could see each individual group of changes, in case you were curious, wanted to try anything out in isolation, or wanted to drop any of them before merge, and also just to explain in at least some detail what I was doing.

ReSharper's formatting works in a much more granular way than VS native stuff, and there are multiple concepts.

The main ones, which are where nearly all of the configuration I put in live, are:

• The Formatting Style and Naming Style sections, which define most of the actual style preferences for non-critical (to the language) format stuff such as whitepace, line breaks, names, etc.
  • These do not do anything unless in combination with other features. They lay out the rules and then you can configure R# to perform related operations anywhere from totally manually to in real-time, as you type and complete expressions and statements in code.
• The Syntax Style section, which contains actual rules for inspections.
  • Many of these correspond to general sections of the other settings, and allow whatever severity you want to be chosen for each.
  • The defaults are pretty lax, and everything you see in the XML file is non-default (since it does not include and will in fact remove default values from the XML).
  • Which inspections I chose to be at which level is somewhat arbitrary, but not just pulled out of thin air, and honestly several more of them could probably be warning or error if we want to strictly enforce certain rules. Contributors can always change the severity on their local config if they want, but would just have to be sure it passes for a PR merge check.

And more, but those are where the majority of the actual important stuff was put in.

For your list, I'll just go bullet by bullet I guess.

• v2_develop: I'm fully on board with that switch whenever you want to do it. I was going to do it myself on my fork, but I wasn't sure if that'd result in PRs looking potentially odd. Probably wouldn't, but I erred on the side of caution. That's set by the default branch setting in the repository settings on github. Once it's made default, it will be the one shown by default on the repo front page and what will be cloned by default when a specific ref isn't given to git.
  • My main cautionary thought is just that, if a non-working version is default, it could be confusing to people who land here from nuget, looking for the v1 source.

• 3192: I've been mostly passively monitoring this one.

  • If it is ready or you're at a good point where it can at least be merged into a branch we create for the big reformat, it'll make life easier.
    • In fact, any PR that is in progress that someone is willing to pause while we do this and then resume afterword would enable us to do this more quickly and safely.
  • Outstanding PRs that are based on anything before the reformat are going to be heinous to merge after it, so we should make sure any that we actually want to merge are either ready to do so or that we are at least aware of them since they'll need updating to avoid too many merge conflicts.
  • Usually, projects/teams that want to do this sort of thing (which isn't THAT common, probably because it's daunting and scares people off the idea pretty quickly 😅) institute a change freeze/moratorium, wrap up important PRs, do the refactor, and then resume from there. Otherwise, the potential for missing things when reviewing merge diffs that are 90% caused by reformatting is not zero. But, tests theoretically should be catching any major code issues, anyway, so there are those and other arguments both for and against. My vote is like
  • A strategy that can be used fairly easily, especially for PRs that don't touch a ton of files or which have pretty concentrated changes, is to go ahead and do the reformat and merge it into the master branch (v2_develop in this case), and then apply the same rules on files touched in those PRs. As long as they look mostly similar, the merge conflicts will be minimal or at least trivial to handle.

• 3181 et al.: Yep, been monitoring those a little bit the last few days, too.

  • Basically the same commentary applies for all outstanding issues with branches/PRs in progress. Though these may have higher potential for merge conflicts, so the strategy I mentioned about about applying the new rules both on the mainline and on those branches before merging them will likely save a lot of time, effort, and premature gray hairs. See below for additional thoughts on this.
• The PR that spawned this, #3207 : Just including for completeness, but the same as above applies.
  • This one is also a good candidate for the thoughts below
• That "fixed build" branch of mine: That was purely for exposition. I'd rather merge the settings branch to v2_develop and then branch from there. Plus, I think a few files might have slightly inconsistent styling because I missed a setting before that was done. It really was quick and easy though, aside from waiting for VS to do its thing, so it's no big deal.

Additional thoughts (mostly github related)

Related to this and various other issues and PRs, I've noticed it's pretty common that we have these lists of associated issues and PRs, and that like 1 out of every 4 or 5 more involved issues ends up having a ton of discussion and scope creep. I would suggest making use of projects for that sort of thing, at the repository level, the organization level, or both (if something spans repositories), to help mitigate that stuff. I know some features are paywalled, but github is inexpensive and I'd be willing to personally sponsor an appropriate subscription for the organization, if the good stuff isn't available with whatever we've got right now.

You also get the kanban boards and that sort of fancy stuff along with that, which is pretty nice for keeping track of work items and keeping things organized. And you get the silly little progress bars showing percentage of associated issues completed, which is fun. 😅 You can also have work items that are not issues OR PRs, which is handy for things that are just ideas someone wants to throw at the wall for later exploration/discussion.

Any issue or PR can be associated to multiple projects, too, with statuses and priorities as detailed as you want (so we don't have to use tags as a pseudo-status and priority system).

So, a few example projects that might be good if you're up for that might be stuff like the following:

• V2 (for anything related to V2 development, obviously
• Code style consolidation/formalization
• Event/command dispatch overhaul
• Responder elimination
• Reflection reduction and AoT readiness

And any other things that are basically categories that apply to more than one work item. And they're an opt-in system, so its pretty darn non-intrusive.

They're easy to set up, too. Just a few clicks and typing a name. Out of the box, they have the basic Todo, In Progress, and Done statuses defined, and adding others is also just a click and typing a name when desired. I typically also have a "Planning" and a "Hold" status as well, to cover those scenarios.

[dodexahedron]

Oh, and I'd also suggest a "VNext" project or similar name, for things we'd like to do, but for a future major or minor release after V2 is RTM

[dodexahedron]

I'm using projects on my fork of TerminalGuiDesigner, if you want to have a look at some basic ones.

This one, in particular, has most of the work I've done and shows a couple more statuses than the 3 you get by default: https://github.com/users/dodexahedron/projects/4

[dodexahedron]

I'm holding off on my other work, for now, pending this, since that's otherwise going to be merge conflict city, given the fact that it'll result in changes to a significant number of files.

Since it's such a core change, I just want to be sure the event stuff doesn't get lost in the shuffle, because of that, which is why I'll wait for now.

[dodexahedron]

After using these rules myself for the past couple of days, and considering what work may result from merge conflicts in the short term from it, I think I want to make a minor change to the settings. Not a format change. Just want to turn the items currently flagged as errors down to warning level, for right now, and then turn them back up to error when things regarding formatting, especially with any work going on in parallel, have stabilized.

The rules will still be the same, and the code cleanup operations will work the same - It just won't block a build if there are violations.

[dodexahedron]

Another thought on how to proceed with the reformatting, when we do it:

Might be best to do it in more than one merge, just for sake of sanity, and to make actual non-structural changes performed by R# more evident, in the following order:

1. Apply the layout rules only, from the "Rearrange Code Only" profile, to rearrange everything.
2. Apply the "Cean Up Redundancies" profile, to do exactly that. It'll remove all the redundant usings, fully-qualified type references, and other redundancies of that nature that have no functional impact.
3. Apply the "Reformat Only - No Sorting or Optimization" profile, to apply most of the actual style rules, excluding a subset of changes.
4. Apply the "Project Standard Cleanup" profile, which will do anything the other profiles didn't do - in particular, it will analyze and apply readonly to fields that aren't written to after construction and make properties get-only, when possible.

The last two steps being separate are the biggest reason for this. It'll make it a lot easier for us to see what actual functional changes are made to the code and then address them, where necessary.

If we do it all at once, there will be basically no visibility into anything without just manually scrutinizing every line of code. Some potential optimizations it can perform at those later stages could affect things that are intentionally written a specific way for the public API surface, since it can't read our minds and know that a given property is meant to be settable, for example.

Future development should make use of the attributes provided in the JetBrains.Annotations package, which include various ways of telling the analyzers what the intent of a given code unit is and adjust its behavior accordingly, solution-wide, for that code and anything that references it. It's not needed in most cases, but sometimes things like a [UsedImplicitly] attribute can be a big help.

[dodexahedron]

Oh and just another general note about this rule set:

This is a starting point, and I'm certain there are additional refinements and enhancements we can make to the shared settings in the future. I have a few thoughts in mind for such enhancements, but this is targeted at the push to standardize the code on the project guidelines in the most evident/profound ways.

There are also features of ReSharper that aren't kept in the dotsettings files, but which can be really nice to have and share, when someone comes up with one. One example is custom Live Templates, which are what they call the transforms that can be applied to a given unit of code through the "surround with" refactorings, primarily. If there's a common operation you perform that isn't covered by a built-in analyzer or refactoring in VS, R#, or a referenced nuget package, these can be hella nice time savers and help keep standard operations...well...standard, without the risks inherent to the otherwise tempting alternative of copy/paste.

[tig]

I'm not sure how to proceed. In the limited time I have right now, I've started reviewing @BDisp's #3181 and found it very challenging because he applied some R# code cleanup. It's next to impossible to tell what was changed to remove constructors and what was changed due to code cleanup and formatting.

I wonder if there's a way for @BDisp to change the R# settings for #3181 to revert back to how v2_develop is formatted/styled without undoing the actual code changes he did?

The biggest problem I'm facing in trying to CR that 158 files touched in that PR is many of the changes are just in white space and XML doc comments. If those were reverted it'd make things much easier.

Another idea is to just say fu*k it and merge #3181 without a code review, trusting that all the work we have to do you list above will get us plenty of eyes on the impacted code. This sounds like a Bad idea, but it occurred to me.

Suggestions?

[tig]

A ton of the merge noise I'm seeing is because of XML doc comment formatting.

@dodexahedron, if we wanted to have R# just reformat the XML doc comments in v2_develop to match the settings below, how would we do that?

In "XML Doc Comments":

These make it so the API doc comments have minimal formatting (with very long lines).

The idea being we could apply this to v2_develop, then to #3181. This will likely simplify the code review for #3181.

Then, later on we could tweak the XML Doc comments settings to have better word wrap and indentation.

[BDisp]

My suggestion is that you perform an R# reformat with Ctrl-E, C and VS2022 Ctrl-K, Ctrl-D on the v2_develop branch and merge before reviewing #3181. I think there must be some way to reformat the entire solution just for all files with the .cs extension. This way I think it would be much easier to check the changes.

[tig]

FWIW, I just discovered https://reviewable.io/

It does a much better job of hiding comment and whitespace changes when doing CRs than github!

I may be able to get throught #3181 quickly this way. I'll keep y'all posted.

[BDisp]

It does a much better job of hiding comment and whitespace changes when doing CRs than github!

But doesn't CI force the conversion of CRs to LFs in files with a .cs extension?

[dodexahedron]

I'm not sure how to proceed. In the limited time I have right now, I've started reviewing @BDisp's #3181 and found it very challenging because he applied some R# code cleanup. It's next to impossible to tell what was changed to remove constructors and what was changed due to code cleanup and formatting.

I wonder if there's a way for @BDisp to change the R# settings for #3181 to revert back to how v2_develop is formatted/styled without undoing the actual code changes he did?

The biggest problem I'm facing in trying to CR that 158 files touched in that PR is many of the changes are just in white space and XML doc comments. If those were reverted it'd make things much easier.

Another idea is to just say fu*k it and merge #3181 without a code review, trusting that all the work we have to do you list above will get us plenty of eyes on the impacted code. This sounds like a Bad idea, but it occurred to me.

Suggestions?

Yeah, that's what prompted my later suggestion of doing it in stages, as separate branches/merges, since it's going to have that effect, given that it's bringing a bunch of disparate styles into one unified style, among other things.

As for what would probably have the biggest impact on the existing PRs, with at least not that much work, is manually putting the members back in the order they were originally in, if they were re-ordered. That, in particular, will confuse almost any diff engine.

On the merge conflict resolution side, specifically... For particularly complex merges, I usually turn to p4merge. It's the diff/merge tool that comes with perforce, and is free. It is, by far, the most powerful and most intelligent merge tool I've found, and does a much better job of handling various things that git or kdiff3 or other tools struggle with.

[dodexahedron]

It does a much better job of hiding comment and whitespace changes when doing CRs than github!

But doesn't CI force the conversion of CRs to LFs in files with a .cs extension?

Yes. It does it in any file it recognizes as a text format, in fact, by default.

Git itself handles endline conversion automatically unless explicitly configured otherwise, which it generally should not be.

Visual Studio and ReSharper also are both capable of handling line ending translation on load, display, save, or any combination of those.

As such, and because line endings being non-native for a given platform can have undesired results, line endings are not really something that should be explicitly enforced at a shared level.

If a given unit of code needs to be cross-platform portable with regards to line endings, the string type has a built-in method for normalizing them for the platform executing the code. String literals should never assume a specific character sequence for line endings.

[dodexahedron]

Another option, for code review purposes, is to look at the individual commits for the relevant changes, and then resolving all conflicts by taking the version from the PR, wholesale.

Yes, if there were actually conflicting changes made in any of those code units, that will clobber them. However, given how fundamentally different the code in the PR is, that was probably largely unavoidable, anyway, and such conflicting changes have a high probability of being incompatible without significant manual validation, anyway. And that'd be just as easy, if not easier, to do by just re-doing any such changes on top of @BDisp's code.

[dodexahedron]

A ton of the merge noise I'm seeing is because of XML doc comment formatting.

@dodexahedron, if we wanted to have R# just reformat the XML doc comments in v2_develop to match the settings below, how would we do that?

In "XML Doc Comments":

These make it so the API doc comments have minimal formatting (with very long lines).

The idea being we could apply this to v2_develop, then to #3181. This will likely simplify the code review for #3181.

Then, later on we could tweak the XML Doc comments settings to have better word wrap and indentation.

It would be simple enough to add a profile that is just XmlDoc formatting, and others that exclude that operation.

But, at a higher level, this actually brings up something that might be worth considering for this project: Separating the XML into separate files, not adjacent to the code. That's a built-in capability of Visual Studio/MSBuild for handling of XmlDoc, and helps avoid this kind of noise.

Here's some documentation on the basic usage of that feature, via the <include> tag: https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/xmldoc/recommended-tags#include

[dodexahedron]

Hey @tig

I did a test merge of your code cleanup branch into @BDisp's constructor killing branch, just to see how crazy it's gotten so far.

Github and even GitKraken have a really hard time dealing with the conflicts, as I'm sure you've noticed.

I loaded up a few of the conflicts in p4merge, with it set to ignore all whitespace differences, and it's handling it so well for the first several files I've pulled up to resolve that I just wanted to suggest use of that, again, if you haven't tried it already. Most conflicted files only end up with 1 or 2 conflicts to resolve, vs dozens in other tools. It's crazy. And the remaining conflicts so far have been trivial to resolve by picking the right source, usually coming down to single-line resolutions.

I knew p4merge was powerful, but this is just too good not to share.

[tig]

I'll look at p4merge asap! Anything can help ;-).

[dodexahedron]

Here's hoping. 🤞

Kdiff3 also handles certain situations better than others, and sometimes I've found myself using a combination of multiple tools for particularly difficult merges, when they're just that bad.

And then there of course STILL often end up being a few things to manually clean up afterward, before the merge is finalized. Usually it's something like slightly inconsistent indentation or a missing curly or something of that nature.

Yippee. 😆

[dodexahedron]

Just noting here for anyone looking for discussion related to this:

The major changes for the actual bulk of the work around this are in #3202 and the latest commentary has been there, most recently.

[tig]

Closing as fixed by #3202.