Discussion: Clarify contributor guidelines

[ThomasGoulet73]

There was some discussion in #9981 about what kind of changes should be included in a pull-request (The main point discussed in #9981 was about unrelated changes for refactoring/formatting).

I took the liberty of opening this issue to discuss it with the community and the WPF team. Obviously, I think we are free to discuss it but the WPF team will have the final say about this.

The end goal of this issue is to document it properly and for the guidelines to be clear to set the right expectation for a PR.

[lindexi]

From my perspective, I believe that @h3xds1nz's contributions are extremely valuable and deserve recognition and encouragement. Although some changes can be extensive at times, as long as they are deemed correct or can be aligned with everyone’s expectations after code review, I think it is not an issue. The only current problem is our inability to promptly process @h3xds1nz's PRs, which has caused me to worry about potential conflicts with my own changes to certain modules. However, this is a minor issue that can be resolved with a bit more caution and patience on my part.

[miloush]

I believe I commented on several occasions to this, so to summarize my views: I prefer matching surrounding code style and doing minimal changes to make PRs easy to review. Using new types and language features is fine by me, renaming members should be justified. I recognize that some people prefer enforcing consistent formatting and I wouldn't oppose introducing this in the WPF repo.

@lindexi too many PRs can be solved by queuing batch of PRs together for a test pass. If there is a good track of these passing, we could become optimistic and increase the batch size (personally I would probably give it a go and try all at once).

[h3xds1nz]

Thanks @ThomasGoulet73 for creating this.

• I think the first step in this would be to coordinate a proper format with .editorconfig included, so that everyone can use standard VS formatting / or dotnet format, and have a nicer experience while contributing. It is a standard practice in large codebases to just press auto format and don't have to worry about a thing.

• Same goes with then resolving the respective issues of using newer language features, such as using nameofs, inlining out variables, use of pattern matching, getting rid of vars; and other stuff that can be done mechanically and improves the code quality by incorporating newer language features that promote safer way to write code / and more readable code.

Those commits then can be ignored in git blame so that they do not pollute the history when looking at versions (that's optional).

I'd even drive this myself if there's an agreement between the community and the team; but I'd hate to see those changes done with a lot of PRs open from all community members because it will then surely lead to merge conflicts on all fronts; and that can lead to errors in the actual submitted PRs, sometimes you just overlook stuff you've previously tested because you've resolved something in a different manner.

There were after all already some changes done in large scale; e.g. string interpolation, usage of throw helpers, SR props instead of constants etc.

@lindexi If you do not resolve merge conflicts caused by your own changes at least 3 times, I do not know what are you doing! 😆

[pchaurasia14]

@ThomasGoulet73 - Thank you for opening this issue.

From WPF side, I agree that we need to update our .editorconfig so that it would enable easier integration with formatting tools. We would ideally like to separate the fixes for styles and code changes so it becomes easier for review and have a better chance of spotting issues.

Updating .editorconfig and enabling analyzers has been on our to-do list for sometime now. We will try and get this fixed sooner.

(It'd be easier to update these style fixes (in-line with other .NET repos) in assembly-wise fashion (similar to @ThomasGoulet73 SR changes) Community suggestions/contributions on this space are welcomed)

Given our performance prioritization, we are okay with ingesting micro-optimizations as it would help keeping the codebase up to date with newer/more performant .NET abstractions. That being said, each of these PRs do undergo full WPF testing to minimize risk of regressions.

[ThomasGoulet73]

In response to @h3xds1nz here #9981 (comment):

Just very recently for example; #9882 - nobody said a thing. All those null-conditional access operators instead of if-checks, changes to the property styles, all it could have been was removing Value and new XYZ. Hell, even reviews from WinForms guys requested to change props/defs to a certain style as nitpicks! Per Jeremy's words

lonitra's suggestions are nits that I'll take care of in the other PR's I'm working on

Guess we're gonna see some stylistic changes from Jeremy in the parts he touches.

Some picks from the PR but there's about 400 of unrelated style changes (null-conditional is the same IL):

https://github.com/dotnet/wpf/pull/9882/files#diff-da7ede7693924ac2f3fa74ffddce4e6980bae053a6b5efccde060f46a26d1d57L1680

https://github.com/dotnet/wpf/pull/9882/files#diff-caf817454a7cc2236a8974f6210284b97dd22b57faee7dc4bf40d69433d0f7c3L2984-L2993

@ThomasGoulet73 Have you been to Winforms repo?

https://github.com/dotnet/winforms/pull/11852/files https://github.com/dotnet/winforms/pull/11843/files https://github.com/dotnet/winforms/pull/11651/files https://github.com/dotnet/winforms/pull/10944/files (This one is particularly juicy in style to changes ratio!)

And there's around hundreds of PRs that are just "simplify"; because format has already been done. Terrifying, indeed. https://github.com/dotnet/winforms/pull/11050/files https://github.com/dotnet/winforms/pull/11044/files https://github.com/dotnet/winforms/pull/11030/files

There's one fundamental differences between runtime - WF (or even aspnetcore) and WPF, that is; WPF ain't formatted, like at all. It didn't get any global polishing unlike other repos. You have statements that are in the middle of the screen and then 1 line put fully to the left. No consistency. So yes, it's super easy to "make" those changes.

My comment #9981 (comment) was mainly about PRs from external contributors. Jeremy is part of the WinForms team and my suggestion wasn't about Microsoft employees. I personally wouldn't have done #9882 the same way but it's not my place to say. A lot of the WinForms PR you included were specifically about refactoring (And also done by Jeremy, a member of the WinForms team).

Maybe I wasn't clear in my comment but I'm in favor of refactoring/formatting-only PRs from external contributors. My point was about not mixing performance enhancement, bug fix or new features with refactoring/formatting unecessary for the PR. Also, the bar for refactoring/formatting changes should be higher for external contributors since IMO it shouldn't be about personal preference, it should be about code that is explicitly against the coding style guideline. Some things of the top of my head that IMO should be accepted as external contributions would be removing unused namespaces.

The last bullet point here says "DO NOT submit pure formatting/typo changes to code that has not been modified otherwise." so technically some of the code in this PR is against the guideline.

Well it's easy to comfort that rule because when you press auto format, nothing changes, since the codebase has been consistently formatted, isn't it. And there aren't 200 suggestions on how to change the code with newer features (that you're accustomed to writing most of the time) because they're incorporated already.

That might be true but IMO that's an argument towards not doing unrelated changes. If you want to fix the format in a PR not about formatting you'll format a lot more unrelated code in this code base than in other code base where you might change a couple of things.

I don't do this part because I enjoy it; I do it in smaller files so I can orient in the code. And I cannot stress how many times I just press dotnet format (because I'm used to having it accustomed to the code base) and I get as many changes as there are lines in the file and then I work through history to revert it if I don't realize it. Just the other day, I was doing some stuff around simple lines:

https://github.com/dotnet/wpf/blob/cbbd92cde6a3c1103ab71fe5c453ec8e017bfb7d/src/Microsoft.DotNet.Wpf/src/PresentationCore/MS/internal/TextFormatting/SimpleTextLine.cs#L88-447

It takes 300 lines for 4 functions, the function Create can be written within 70 lines with the comments, using standard conventions, with brackets, while it currently takes 160 lines. After format it fits into a normal 16:9 monitor with a full HD resolution (rather common) and the lines do not cross 150 characters ever (it's around 130 with the longest one, usually 120 is the sweet spot and recommended iirc), which is the recommended max line length you'd want at any circumstances.

And then you can finally understand and remember/see what it does without scrolling up and down. Writing short and oriented functions/procedures is not just about principle but also about readibility.

Personally, I think the code you linked is "fine". I probably wouldn't have written it the same way but I would be comfortable working in it. In the "70 lines" you talk about I believe you would put stuff on a single line instead of multiple lines which IMO would hurt readability. A single line shorter than the recommended max length doesn't mean that it is more readable than being on multiple lines. Less lines != more readable.

Oh and this function of 22 lines? Tell me what is the edge case. None actually.

https://github.com/dotnet/wpf/blob/cbbd92cde6a3c1103ab71fe5c453ec8e017bfb7d/src/Microsoft.DotNet.Wpf/src/PresentationCore/MS/internal/TextFormatting/SimpleTextLine.cs#L403-425

You bet I'd rewrite it in 10 (or 6, if you remove the braces), so you don't have to think about it:

private static void CollectTrailingSpaces(List<SimpleRun> runs, TextFormatterImp formatter, ref int trailing, ref int trailingSpaceWidth)
{
    for (int i = runs.Count - 1; i >= 0; i--)
    {
        if (!runs[i].CollectTrailingSpaces(formatter, ref trailing, ref trailingSpaceWidth))
        {
            break;
        }
    }
}

Let's say you were opening a PR about changing ArrayList to List<>, why change the formatting if it's not explictly against the coding style ? What makes your preference better than the preference of the previous author ? Also, your suggestion would throw a NullReferenceException if runs was null, something that the previous code handled. I'm pretty sure you didn't spend a lot of time on it and maybe you would've made it differently if you opened a PR with this but I hope you understand the point I'm making.

Lastly, I'd be happy to do the same thing WF and runtime did (address formatting and newer compiler features etc. globally) to reduce noise in perf PRs if its discussed properly with the community and the team. It would make my life easier (and anyone else's contributing) as well to not have to care about those things, that's a sure bet. It would get the codebase to a much better state too.

I share the same opinion.

I tried to answer all your points but let me know if I missed one.

TL;DR:
I don't think external contributors should format code they didn't write using their personnal preference, it should only be when it is explicitly against coding guidelines (Or when discussed with the WPF team and/or the community beforehand). I also don't think PRs should contain unrelated changes (Performance/bug fix PR shouldn't contain formatting changes and vice-versa). Formatting PRs should be for one or few changes (Like "Removing unused usings"), be properly scoped (Whole repo /single project/single folder/single file/etc) depending on the size of the change and ideally be automated (Like with VS format or dotnet format) though that's not always possible.

[h3xds1nz]

My comment #9981 (comment) was mainly about PRs from external contributors. Jeremy is part of the WinForms team and my suggestion wasn't about Microsoft employees. I personally wouldn't have done #9882 the same way but it's not my place to say. A lot of the WinForms PR you included were specifically about refactoring (And also done by Jeremy, a member of the WinForms team).

Maybe I wasn't clear in my comment but I'm in favor of refactoring/formatting-only PRs from external contributors. My point was about not mixing performance enhancement, bug fix or new features with refactoring/formatting unecessary for the PR. Also, the bar for refactoring/formatting changes should be higher for external contributors since IMO it shouldn't be about personal preference, it should be about code that is explicitly against the coding style guideline. Some things of the top of my head that IMO should be accepted as external contributions would be removing unused namespaces.

I don't see how the bar should be different, when working with a community of people, the rules should be same for everyone. But it's not enough to set rules if the codebase sits in the state it currently is. Don't remind me removing unused namespaces, last time when that PR got merged from Jeremy, I had to fix 17 PRs conflicting; if the PR with removing unused namespaces goes through, I'll have to fix most of my PRs which I really don't wanna do. Kind of don't wanna post anymore right now until I know what happens.

That's another thing, I've said it in the previous post but I'll say it again, I'd hate to see those changes done with a lot of PRs open from all community members because it will then surely lead to merge conflicts on all fronts; and that can lead to errors in the actual submitted PRs, sometimes you just overlook stuff you've previously tested because you've resolved something in a different manner.

And yes, I'm mostly talking about myself with the number of PRs open.

That might be true but IMO that's an argument towards not doing unrelated changes. If you want to fix the format in a PR not about formatting you'll format a lot more unrelated code in this code base than in other code base where you might change a couple of things.

The argument here is that I'm used to press auto format after every line I write, it's something I've learnt years ago when we switched to auto formatting, there's just nothing better than consistency and not having to care about it when it's done automatically.

I have to often write the code in a separate project to prevent this "habit" from striking in big files, it is what it is. That's my problem, obviously, but just sharing my experience at this point.

Personally, I think the code you linked is "fine". I probably wouldn't have written it the same way but I would be comfortable working in it. In the "70 lines" you talk about I believe you would put stuff on a single line instead of multiple lines which IMO would hurt readability. A single line shorter than the recommended max length doesn't mean that it is more readable than being on multiple lines. Less lines != more readable.

IMO having the whole method at screen size, knowing from what happens from top to bottom is way better than having to scroll 5 times up and down when you're going through the code. I know less lines != more readable, I often e.g. add braces to if/foreach/for/while/etc. blocks with 1 statement when I feel it's more readable (typically a longer statement) or simply when you need a separation. We can agree on that.

Let's say you were opening a PR about changing ArrayList to List<>, why change the formatting if it's not explictly against the coding style ? What makes your preference better than the preference of the previous author ? Also, your suggestion would throw a NullReferenceException if runs was null, something that the previous code handled. I'm pretty sure you didn't spend a lot of time on it and maybe you would've made it differently if you opened a PR with this but I hope you understand the point I'm making.

The hidden point there is that runs cannot be null, it is called only in 1 spot in the codebase from a method that initializes the collection. And it's just actually from one of many PRs I'm yet to post (and I've spent time on, yes). Everything requires testing so I take my time with it before posting.

TL;DR: I don't think external contributors should format code they didn't write using their personnal preference, it should only be when it is explicitly against coding guidelines (Or when discussed with the WPF team and/or the community beforehand). I also don't think PRs should contain unrelated changes (Performance/bug fix PR shouldn't contain formatting changes and vice-versa). Formatting PRs should be for one or few changes (Like "Removing unused usings"), be properly scoped (Whole repo /single project/single folder/single file/etc) depending on the size of the change and ideally be automated (Like with VS format or dotnet format) though that's not always possible.

A lot of the times it is possible, it just haven't been done yet. With scoping I couldn't agree more but then again, earlier point about conflicts stands.

I agree less with perf PRs (again, if there was proper formatting, its less noise to begin with) but I fully agree with bug fixes. All the bugfixes I have open are just the necessary lines when possible since changing (fixing) the behavior is something that should be left without any other changes.