search

microsoft / WinUI-Gallery

This app demonstrates the controls available in WinUI and the Fluent Design System.
MIT License
2.81k stars 628 forks source link

Discussion: Squiggles, Rosyln Analyzers, and EditorConfig files #331

Closed kmgallahan closed 4 years ago

kmgallahan commented 4 years ago

I'd like to open a discussion about squiggles & suggestions in the codebase, use of Roslyn analyzers, and EditorConfig files.

I'll admit, I'm a stickler for stamping out squiggles (and suggestions) in my own codebases. I even go further and use multiple analyzer packages to add more rules on top of the basic VS ones.

The WinUI repo has contribution guidelines, which I'd presume also apply here. Those guidelines have a code style and conventions section which reference the .NET Core team's C# coding style rules, and the .editorconfig file used for that codebase.

This repo does not have a top level .editorconfig file, and I think that should be the first step in any process to get a wrangle on VS squiggles & suggestions.

Although the microsoft-ui-xaml / WinUI .editorconfig file could just be adopted, the .NET Core version is also an option.

Including the EditorConfig file would just be the first step. I'd then like to slowly start working through some rules to resolve inconsistences in the codebase (without stepping on any toes of course), while also tweaking the config where necessary.

Later, I'd potentially like to use this process with other MS WinUI-related open source codebases, such as:

I wanted to start here though because I feel the XAML Controls Gallery is a great example of a semi-complex app with a codebase many newcomers likely dip their toes in to see how to get things done. It is also less sensitive to PR merging than the WinUI repo.

Fewer squiggles and less inconsistencies in an official codebase that gets a lot of eyes would then help encourage proper use of analyzers and EditorConfig files elsewhere.

Let me know your thoughts.

marcelwgn commented 4 years ago

While I agree in your opinion that we should clear the projects from warnings and squiggles, I do not think that this will be a fun job.

First of all if you include "Messages" as Visual Studio calls them, there are over 400 things to clear in the XCG alone. Some of these might be easy to clear, some of them might be more tricky and some of the might be very annoying as they occur very often.

Having a .editorconfig file might be good, however I think ignoring warnings in a Directory.Build.targets in addition to editorconfig might be better, since the CI will still report warnings if we only use editorconfigs.

I am also a fan of additional code analyzers, but which code analyzer should WinUI projects choose? In my project (warning: shameless plug ahead) https://github.com/chingucoding/UWP-Resources-Gallery I have used FxCopAnalyzers.

While mostly useful, it seemed to warn me about issues that were none in my case:

And those are issues that will also be present in XCG and the other projects you mentioned. Should we suppress them globally? Or just in file and bloat up the source files?

Nonetheless, I think some consistency across projects might be good, though I think this will be a lot of work and very time consuming.

kmgallahan commented 4 years ago

I do not think that this will be a fun job. I think this will be a lot of work and very time consuming.

No worries, I'll take the brunt of it as I don't expect most people care about this type of thing. Merging PRs is the only thing I can't do and may be tricky on the MS side.

However, I'd keep each PR well-scoped and branched from the latest commits so it shouldn't be painful.

One question on this topic that I have specific to the XAML Controls Gallery (@stmoy @YuliKl) - is the plan to merge the WinUI 3 branch back in with the WinUI 2 one, or keep them separate and make the former the master at some point?

there are over 400 things to clear in the XCG alone. Some of these might be easy to clear, some of them might be more tricky and some of the might be very annoying as they occur very often.

Part of the strategy is to over time decide on which analyzers should produce suggestions at all. Making some editorconfig tweaks can result in dozens to hundreds of items going away at once (for everyone touching the codebase too).

For the ones that will require getting dirty, I'm ready. After all I'd rather do this than start on step 1 for contributing to the WinUI codebase: re-learn C++...

which code analyzer should WinUI projects choose?

I'd just like to tackle the built-in VS analyzers first (IDE****). I'd think the next most sensible package would be FxCopAnalyzers as you mentioned, as even VS pops a recommendation to install them once in a while. This would in the fairly distant future though, depending on the rate of progress.

Regarding specific analyzers - I can open new issues to tackle ones where discussion might be needed, either individually or in groups. An example of a group would be all of the "unused" warnings - I find them quite annoying, especially in a codebase that is under active development, so I'd just have them shut off in the .editorconfig file.

Or just in file and bloat up the source files?

I dislike use of any in file suppressions, so they'll be avoided at all costs.

I would definitely like to give this a go by just starting here in the Gallery to see how things progress. Other MS open source repos can get attention if this goes smoothly, and they'll benefit from having a well-defined EditorConfig + suppressions file (if needed) from the start. Plus discussions about usage of specific analyzers will already be out of the way, or at least have justification documented.

marcelwgn commented 4 years ago

No worries, I'll take the brunt of it as I don't expect most people care about this type of thing. Merging PRs is the only thing I can't do and may be tricky on the MS side. However, I'd keep each PR well-scoped and branched from the latest commits so it shouldn't be painful.

If you want to do this, sure! Also if you like I can try to help you, since I would also prefer projects to have an empty warning list. 😁

Part of the strategy is to over time decide on which analyzers should produce suggestions at all. Making some editorconfig tweaks can result in dozens to hundreds of items going away at once (for everyone touching the codebase too).

Yes that's right. However I mostly try to not ignore warnings and such because what if you actually have a case where the warning is valid, but I generally ignore those warning because they were annoying? E.g. Unused functions . If you ignore this warning totally you will likely not notice that a function is obsolete, since VS won't tell you.

I'd just like to tackle the built-in VS analyzers first (IDE****). I'd think the next most sensible package would be FxCopAnalyzers as you mentioned, as even VS pops a recommendation to install them once in a while. This would in the fairly distant future though, depending on the rate of progress.

Yes, IDE**** warnings should be the priority. FxCopAnalyzers is something we could do later, though in some cases it may be a bit overkill in my opinion (see all the warnings related to strings).

Regarding specific analyzers - I can open new issues to tackle ones where discussion might be needed, either individually or in groups. An example of a group would be all of the "unused" warnings - I find them quite annoying, especially in a codebase that is under active development, so I'd just have them shut off in the .editorconfig file.

Maybe this is something that should be discussed once VS warnings and messages have been cleared. 😅

I dislike use of any in file suppressions, so they'll be avoided at all costs.

Yes I feel the same. However some warnings do not make sense to ignore globally, so what should do with that?

For the ones that will require getting dirty, I'm ready. After all I'd rather do this than start on step 1 for contributing to the WinUI codebase: re-learn C++...

I can see that C++ is a bit off putting. However I have not learned C++ (only a bit of C). The WinUI repro really takes away a lot of the C++ overhead by already providing functions for a lot of things. Also if you don't know something or if you are not sure, so far the great people at WinUI always were happy to point me to the right function to use or the right file to look for an example.

Firstly I also had the opinion, I would barely be able to tackle any issues since my C++ skills were/are non existent, but once you look at a bit of the existing source files, you can slowly see how the components work (and how problems are solved).

I hope that the above does not sound arrogant or anything like that, that is definitely not my intention. Also if you like I can help you get started with WinUI issues, and again I am sorry if I come of as arrogant, I just want to help fellow developers. 😅

kmgallahan commented 4 years ago

I mostly try to not ignore warnings and such because what if you actually have a case where the warning is valid

Specifically regarding the 'unused' analyzers, I'd have a comment in the EditorConfig suggesting that the lines be commented out if you want to check the solution for unused members during a code cleanup session. Otherwise you're constantly being nagged while in the middle of writing methods, or that your library's public API members aren't being used... yah, its a lib that hasn't been consumed yet...

Maybe this is something that should be discussed once VS warnings and messages have been cleared.

I'm talking about individual analyzers themselves (not packages) and how they should be handled. A prime example: IDE0003: Name can be simplified. There are almost 300 occurrences of .this in the codebase, yet the guidance states:

  1. We avoid this. unless absolutely necessary.

So before just sending a PR to remove all ~300, I'd first want to know if that is the right direction, if there are specific exceptions to consider, or that the maintainers would rather just turn IDE0003 suggestions off (for some reason, hopefully a good one...).

some warnings do not make sense to ignore globally, so what should do with that?

Something to tackle towards the end once the low hanging fruit as been harvested. 😅

Regarding the C++ comments - no arrogance detected! I'm sure it wouldn't take me that long to dive in. It is more a combination of

marcelwgn commented 4 years ago

Specifically regarding the 'unused' analyzers, I'd have a comment in the EditorConfig suggesting that the lines be commented out if you want to check the solution for unused members during a code cleanup session. Otherwise you're constantly being nagged while in the middle of writing methods, or that your library's public API members aren't being used... yah, its a lib that hasn't been consumed yet...

That would be a good compromise for warnings that partially trigger rightfully and partially detect non issues.

I'm talking about individual analyzers themselves (not packages) and how they should be handled. A prime example: IDE0003: Name can be simplified. There are almost 300 occurrences of .this in the codebase, yet the guidance states: [...]

Oh I see, sorry about the confusion on my side.

So before just sending a PR to remove all ~300, I'd first want to know if that is the right direction, if there are specific exceptions to consider, or that the maintainers would rather just turn IDE0003 suggestions off (for some reason, hopefully a good one...).

Yes that is a good idea! And it's great that you created this issue where we can discuss such things!

Regarding the C++ comments - no arrogance detected! I'm sure it wouldn't take me that long to dive in. It is more a combination of

  • Not having used the lang in forever
  • No idea how much code churn is happening in the background for the WinUI 3 release that would overwrite changes made today
  • No access to a bunch of the source code that would be needed and/or help tremendously in working through issues

I can definitely understand your reasoning, after all we currently don't know how WinUI 3.0 will look source code wise, so some things may change. However I doubt that, since it would be very inefficient to throw away bug fixes that have already been made.

The last thing you mentioned is a very good point. Currently a lot of the issues (I think around 40%-60%) of bugs can't be fixed as they are outside the scope of WinUI 2.0 . With that said, there are also a lot of issues which can "easily" fixed without WinUI 3.0 (meaning that having WinUI 3.0 won't make it easier). Some issues which are a good start are issues tagged with Good first issue and help wanted. After all, every issue fixed is one issue less WinUI users/developers will encounter.

Maybe this are things which also should be discussed over at the WinUI repo.

stmoy commented 4 years ago

My totally non-comprehensive reply to this issue: I have no objections to cleaner code or fewer squiggles. If this is something that you're willing to tackle, we will help facilitate PRs/merges.

One question on this topic that I have specific to the XAML Controls Gallery (@stmoy @YuliKl) - is the plan to merge the WinUI 3 branch back in with the WinUI 2 one, or keep them separate and make the former the master at some point?

@jesbis can provide more context on the longer term plans, but the current plan is that both WinUI 2 and WinUI 3 will live alongside each other for some time, and during that time we plan to have concurrent versions of the Gallery. The WinUI 3 alpha version of the Gallery is updated during major updates to WinUI3 and is made by taking a fork of the MASTER branch of the Gallery and not by trying to keep the WinUI3 Gallery updated alongside the WinUI2 Gallery.

Once WinUI2 is superseded wholly by WinUI3, we'll merge and have a unified gallery at that point, but that's a long ways out.

TL;DR - work done to the MASTER branch of the Gallery is good and valid both in the short term and the long term, because the WinUI3 version of the Gallery is created by taking forks off of MASTER.

kmgallahan commented 4 years ago

Thanks for the info. One more important item to tackle before I get started.

The long term goal is to develop a robust .editorconfig file that can be used by other MS open source projects, as well as 3rd party code. It would be especially helpful for new folks that are a bit overwhelmed by all the available analyzers, let alone how to configure each one.

Next to the configuration line for some analyzers (IDE0003 for example) I'd like to be able to reference github issues that provide justification and a place for discussion/feedback. Rather than create those issues in this repo (as it'd be odd pointing here once the file is used in other projects), I think there should be a separate repo just for this purpose.

It could be called something along the lines of "Roslyn Analyzer Analysis". I could host it, but it might be more suitable for it to live under github.com/microsoft instead.

Your thoughts? @jesbis @stmoy @chingucoding

marcelwgn commented 4 years ago

Having a platform to discuss such .editorconfig files seems like a good idea. However I am not quite sure how general a editorconfig file can be, i.e. how easy it would be to have them in multiple repos if they can be shared at all, since some repos have different programming languages and standards as others.

Next to the configuration line for some analyzers (IDE0003 for example) I'd like to be able to reference github issues that provide justification and a place for discussion/feedback. Rather than create those issues in this repo (as it'd be odd pointing here once the file is used in other projects), I think there should be a separate repo just for this purpose.

Maybe such discussions should be kept in the repos, since the cases in which IDE**** was triggered may be different, and in some cases its valid and it some its not valid, depending on project and context.

kmgallahan commented 4 years ago

Ok, so I spent some time working through the various aspects of this. And by some time I mean more time than I should have.

I wanted to answer these questions:

and quite an important one:

To that end I created styleascode.net.

It's hosted via GitHub Pages - repo link

Note: the site is obviously a work a progress, and much of the content will be driven by how things go here. It's also just statically generated, so nothing much fancy going on - just a bunch of markdown pages. Details in the repo readme.

I went ahead and:

I also created a PR to get the ball rolling by adding the minimal .editorconfig to this project, and by addressing one rule violation. These used the templates found here.

So yep... that happened. Feedback and contributions are welcome.

If the PR and accompanying .editorconfig is accepted then I'll proceed to resolve more rule violations following this process.

kmgallahan commented 4 years ago

@chingucoding

To expand on something I mentioned above and in the PR notes - this minimal config is only meant to be a starting point.

Once it is finalized and those rules start to get resolved I'd like to switch to the non-minimal version here which will grow with new rules as they are agreed upon.

One of the first noticeable changes in that config is the togglable rules section for unused code - which will eliminate hundreds of suggestions from the Error List once adopted.

marcelwgn commented 4 years ago

Once it is finalized and those rules start to get resolved I'd like to switch to the non-minimal version here which will grow with new rules as they are agreed upon.

There does not seem to be a huge difference between your link and the PR (around 30 lines of code). Is that intentional? 😅

One of the first noticeable changes in that config is the togglable rules section for unused code - which will eliminate hundreds of suggestions from the Error List once adopted.

That's a great idea! 👍

kmgallahan commented 4 years ago

So when I first started on this ~a week ago the non-minimal version included all the rules defined in the .NET Core runtime team's EditorConfig, so there were many more.

However, once I started doing research on how agreeable rules were and did the comparison with the compiler team's EditorConfig I realized that just dumping ~150 rules into the config without thinking through and justifying all of them probably wasn't the best approach.

Hence:

  1. Start with minimal EditorConfig
  2. Agree on starting point and resolve some violations
  3. Move to non-minimal EditorConfig
  4. Begin adding additional rules 1-2 at a time with proper justification (minimal config is no longer touched at this point)

This should result in a good ruleset that is agreeable to everyone and should be usable in other projects.

kmgallahan commented 4 years ago

This work is now underway.

Closing this issue in favor of one dedicated to work-item tracking: #363.