General using cleanup in C# files #10011
Conversation
dotnet-policy-service
bot
requested review from
dipeshmsft and
singhashish-wpf
dotnet-policy-service
bot
added
the
PR
JeremyKuhne
force-pushed
the
cleanusings
branch
from
66b7078
to
f191868
Compare
|
@Kuldeep-MS CodeFlow can actually open the whole thing, it does take a while for it to finish loading all of the diffs. |
|
This feels like merge conflicts on 90 PRs. |
If they all touch the usings, maybe, but hopefully not |
| using MS.Win32; | ||
| using MS.Internal.PresentationCore; | ||
|
|
||
| namespace MS.Internal.AppModel | ||
| { | ||
| static class CookieHandler | ||
| static class CookieHandler |
There was a problem hiding this comment.
Couple of files have this change, doesn't seem intentional or desirable
There was a problem hiding this comment.
They should be indented like that, the tooling did it. There is a meta issue of indenting the types properly in some cases.
There was a problem hiding this comment.
Yes but it doesn't indent the whole class, just this line? (+leading trivia)
| // | ||
| // Spec: Text Formatting API.doc | ||
| // | ||
| // |
There was a problem hiding this comment.
Why are we removing these?
There was a problem hiding this comment.
There's quite a good amount of the comment descriptions in headers removed. That should be rectified.
There was a problem hiding this comment.
I removed what wasn't useful. The comments were either repeated in the class description right below, or referring to documents that are not publicly (or otherwise) available.
There was a problem hiding this comment.
Thanks for the clarification, I am guessing with manual editing you easily miss things (e.g. EventMap.cs has the description removed but EventPropertyMap.cs not despite very similar class comment, several files still have document references).
My biggest concern is removing references to documents that the team could find useful should they need to get familiar with the code, even if they are not available publicly.
There was a problem hiding this comment.
@miloush I understand your concerns. We have multiple ways to find these things internally if we need to. Full disclosure, however, is that sometimes documents get lost after a few decades. :) Thankfully a number of these seem to have been checked in internally.
|
@JeremyKuhne can you give an example of the CsWin32 issue this is trying to solve? Does it generate different namespaces in one file? |
Well, I wish. Unluckily it is the case. |
@miloush As I mentioned in the description, all of the Windows APIs start with The other goal here is part of facilitating contribution momentum. When code isn't consistent or meeting coding styles it can make reviewing changes much more time consuming and difficult. Time spent going back and forth explaining coding style that could be enforced with tools is wasted time. I realize that this and some of the other changes I have pending will require resolving some merge conflicts (apologies in advance), but the ultimate goal is accelerating long-term contribution momentum. I have a change I'm working on that broadly enables Xunit tests and the Test Explorer experience as a critical part of this. There are follow ups that will consume OLE implementations (Clipboard/Drag & Drop) from WinForms (said code is AOT friendly). I've also got changes that lay the groundwork for using CsWin32 and ComWrappers for the rest of WPF. I want to see WPF moving faster ultimately. That requires enabling test infrastructure and modernizing/cleaning the codebase. Leaning in aggressively on these has paid off in practice for the WinForms repo- I encourage looking at https://github.com/dotnet/winforms/graphs/contributors (where the top 2 contributors are not MS employees). Also note that now is the best opportunity to make these sort of changes. If you have PRs that are effectively non-functional or low risk feel free to ping me directly on them so I can comment. I've already helped unblock a few. My first priority is getting the test change committed so PRs can include unit tests, but I will do my best to help when I'm able. |
|
@JeremyKuhne I just want to point out that you are dropping this in the midst of heated discussions in #9981 and #9994 |
|
@JeremyKuhne Thanks for explaining the CSWin32 part. While I'm super happy with the fact that we're heading towards faster momentum on WPF (that's a child's dream coming to life), the support of unit tests is a nice promise too for a more modern testing infrastructure and ensuring correctness, I can't say how disappointed I would be if I had to resolve the conflicts on that number of PRs, especially when like 30 has already went through the test pass and just awaits actual final team member review before merging. I had enough doing that after merging the long overdue CAS SecurityClass/Value/etc. and that was just 16 PRs affected but I've spent over an hour on that tedious process (who likes resolving merge conflicts, right) and actually even then I've made 2 mistakes I had to fix eventually when reviewing it again. And for myself I can say that if I had to now resolve the merge conflicts that would hit over 60% on my open PRs (probably like 70% of them which is like 60 PRs), I'd be pretty disgusted to contribute again and "(apologies in advance)", while a genuine apology from your side, won't cut it for me, especially since most of them are pretty straightforward adjustments too. I'm happy to NOT contribute publicly for a while we're just getting the codebase into good health (or help in that regard), that is totally fine but otherwise I'd feel pretty screwed over at current state.
@miloush has already said this in dotnet/winforms#12362 but why are those changes not discussed or at least introduced to the community to have a chance to talk about it in the WPF repo? While I welcome the fact there's a push for change, I don't like the way it's being done. Sure, WPF had probably like 4 total API additions since it was opensourced, rest are just proposals that never got to API review but if the goal is for WPF to start moving at a faster pace, we'll maybe get there too :) |
@h3xds1nz mistake on our part, we should have opened an issue in WPF that linked to ours to make it more obvious. We talked with the internal team and overlooked following up with community. We'll do our best to create linked issues in the future.
I understand the frustration and if there is anything I can do to help with reviewing, please let me know. I feel your pain, and I encourage you to ping me directly for things. I'm not on the WPF team, but I am the architect for WinForms and my assessments should make it easier for the WPF team to sign off on things. The fact that you have so many open PRs is an indication that we're not in a great place. If we want to make significant strides here, now is the time to make broader code changes (before Preview 1). It doesn't mean we can't take existing things into account and flex somewhat, but waiting for everything to clear will lose the opportunity, both with the overall schedule window and my personal ability to directly contribute. Fwiw, I have the tests working, even for WindowsBase. I'm just trying to make sure everything is working smoothly before I create the first PR. The last thing I'm tackling is an AnyCPU issue. We can't really build things as AnyCPU from a test perspective as there are always native dependencies. You can manually address this, but I want it to be completely seamless. You should be able to |
|
@h3xds1nz doing a cursory look at some of the PRs it is pretty clear that getting unit tests up and running would be hugely beneficial to greasing the wheels here. I'll do my best to get my PR enabling unit tests in the next few days. |
@JeremyKuhne Thank you. Looking forward to it.
I guess you'd need to discuss this with the team how you could help in this regard, I unfortunately don't know how you can help to push things faster so I'm not left with resolving merge conflicts as a full time job, haha. I agree it is an indication that things are not ideal and I obviously understand that we're on the clock here to get stuff out before preview 1, as always. I'm not expecting for everything to clear out, of course, as I've mentioned, I'm happy to help in this regard anyhow.
Yeah, it would be hugely beneficial; I've mentioned it months ago and greasing the wheels is the perfect expression for it. I was also writing parts for the public surface where I knew no tests existed, so it's gonna be great those get their use as well outside my personal testing. With that being said, thank you for your interest and trying to move things in the right direction. |
This removes unnecessary using statements and moves them outside of the namespaces when they were inside them. I cleaned up the headers a bit for several hundred files, but the cleanup there isn't exhaustive by any means. Changes are only to the tops of files. To make this work a bit more cleanly I've added GlobalUsings.cs files to some of the assemblies.
JeremyKuhne
force-pushed
the
cleanusings
branch
from
f191868
to
aa650a7
Compare
|
Rebased |
This removes unnecessary using statements and moves them outside of the namespaces when they were inside them.
I manually cleaned up the headers a bit for several hundred files, but the cleanup there isn't exhaustive by any means. Changes are only to the tops of files.
To make this work a bit more cleanly I've added GlobalUsings.cs files to some of the assemblies.
NOTE: This was done using the code tools in Visual Studio.
Having usings inside of namespaces makes it difficult to use CsWin32, which uses the published Widows metadata names. Unsurprisingly, namespaces all start with
Windows.which doesn't resolve when trying to do a using inside of a namespace definition whereWindowsis part of the name.Finally, there are three HRESULT implementations in WPF, which are all visible in some assemblies. I had to put a couple of static usings inside of namespaces to resolve this. Moving to CsWin32 we can use the generated definition and avoid the conflicts.
Microsoft Reviewers: Open in CodeFlow