Feedback on contributing

[GeorgeLeithead]

After my first experience on contributing, here are my thoughts and personal opinions. @bijington has also asked for my feedback.

The community

The community members were really, really helpful in providing support and guidance. Cant thank them enough. Thanks to @brminnick, @bijington, @pictos and @VladislavAntonyuk for all their reviews and help with the code and documentation.

GitHub

Most of my problems and issues were around GitHub and Git. Not every developer is familiar with using GitHub, Git, Forks, branches, Fetch/Pull/Push, etc. personally, I've been used to being the only (or being in a small team) developer, and I haven't used a lot of GitHub features in anger.

A brief guide as to community toolkit process (along with GIT commands & Visual Studio equivalent) would really help, even if it's a link to a blog article (or one of the famous @jfversluis YT videos). It shouldn't be re-inventing the wheel, but a reference guide and shortcut for new contributors, who may not be all too familiar with GitHub and Git.

For example,

• How to create a Fork and a branch (and WHY do you need to do this?)
• How to keep your fork and branch up to date with the upstream. (and WHY do you need to do this?)
• How to initiate your first push, and what to expect.
• How does the review process work? Do I comment? Do I mark as resolved? When should I request a review?
• etc.

Tooling/Rules

I love the fact that the project includes .editorconfig and in my opinion is great practice! However, there were clearly some problems and issues I repeatedly had throught the life span of my contribution. In order to reduce confusion, un-necessary changes (resulting in extra review work), time and effort I suggest that there either needs to be a strengthening of the .editorconfig rules, additional analyzers added, and/or a clear guide as to what extensions/code clean up should be used.

Code Clean up

When I first started, the extensions and configuration of 'Code Clean up' caused quite a few issues and made un-necessary changes to code. These often clashed with the .editorconfig rules, and required that I either manually fixed or changed my 'Code Clean up', which I need/use for OTHER projects. For example, I prefer to use a discard, such as:

Before my code clean up:

services.AddTransient<AlertsGalleryPage, AlertsGalleryViewModel>();

After my code clean up:

_ = services.AddTransient<AlertsGalleryPage, AlertsGalleryViewModel>();

I also prefer to use implicit types, rather than var (perhaps this should be in the contributing guide, along side the 'spaces or tabs'), which also resulted in:

Before my code clean up:

		var builder = MauiApp.CreateBuilder()
								.UseMauiCommunityToolkit()
								.UseMauiCommunityToolkitMarkup()
								.UseMauiApp<App>();

After my code clean up:

		MauiAppBuilder builder = MauiApp.CreateBuilder()
								.UseMauiCommunityToolkit()
								.UseMauiCommunityToolkitMarkup()
								.UseMauiApp<App>();

I had to remove the 'Code clean up' rules, which then caused me problems for my other projects. So, I had to remove the rules from profile 1 (default), and add to profile 2, and TRY and remember to manually select this on my other work projects. For example, dotnet_style_require_accessibility_modifiers = omit_if_default:error really caused me a lot of problems with Code Clean up.

StyleCop

In the review I also had a request Please organize elements in this file according to the following StyleCop rules: but there are no StyleCop rules enforced! So, I temporarily added StyleCop.Analyzers, got it to help me re-organise the code, and then removed the analyser again. Not ideal, but it worked.

Inconsistency

There was also inconsistency for things like 'Block body constructors' or 'expression body for constructors'. I'd implement it as one, someone else would change it to the other, and in another code file they would implement it as the first. Then later change it all again. I still have no idea as to which one should be used (I think 1 should be made the standard and we use that).

Currently in AvatarView there is a bit of a mix between the two, and this is a personal niggle.

Discussions

I'm still not clear as to where I should raise a discussion in regards to a specific contribution. Should it be in the Pull Request Conversations, discussions, issues, etc? I raised a few 'questions' in the pull request conversation, but I feel they ALL got skipped/ignored. For example I raised a question about the rotation property. To expand, we can use the Rotation property along with the CornerRadius property to make a map drop-pin (Rotation=45). However, the Rotation rotates the whole control. I raised in a discussion if we should add an additional property to allow for the rotation of the control content, thus allowing the drop-pin look (Rotation=45) and keep the content rotated correctly (ContentRotation=-45, or CounterRotateContent=true).

Using exiting code as a reference

I used a lot of existing code as a guide, to help me achieve the same high standards already in place. However, there was a couple times where I was asked to make changes to code that I simply bulk copy/paste from other areas in the toolkit. I wasn't sure if I should point out that it was a copy/paste or go and change the source.

For example, in the samples for PoPup, it uses a BoxView used to draw a HR. I copy/paste this into my samples, but was asked to change this for Line. I did point out that this was a copy, but... see discussions above. Plus, I didn't go and change where I copied this from...should I have??? (I would have been happy to do so!)

Release

I must admit, that the release of the feature I was working on came as a total surprise to me. I had absolutely no idea it was coming, and as such there is at least 1 'bug' that I know of, that I had intended to work on, but as the feature was released, It's my fault for not raising in the Pull Request Conversation, but see Discussions above.

I've raised it as an issue AvatarView using StrokeShape and ImageSource image is incorectly clipped and I'll be looking into it

Monthly stand-up

I think the monthly stand-up could be promoted and detailed/advertised in the repository. I will admit that I only stumbled on the stand-up, as I was watching the .NET MAUI stand-up and they mentioned the Community Toolkit stand-up was happening right after! If that hadn't happened, I wouldn't have looked to contribute.

[GeorgeLeithead]

I'd also like to point out that I don't have any iOS or MacCatalyst devices. As such, I was not able to validate any of my contributions on these devices. I was also unsure if anyone had managed to test/validate on these devices. I think there needs to be perhaps a FINAL checklist that validates that it has been tested on ALL platforms, before any merge is approved.

[TheCodeTraveler]

Yup! I tested each platform

[GeorgeLeithead]

PHEW! Glad you did, and I did know you tested on iOS, given the background bug. I do think a checklist somewhere in the process for completness might be prudent.

[TheCodeTraveler]

Lets open an Issue for any bugs/known-issues

[GeorgeLeithead]

Lets open an Issue for any bugs/known-issues

It wasn't a bug, until it went live. Before that, it was a 'requirement', and I was planning on working on it.

[bijington]

Feedback

@GeorgeLeithead thank you so much for this, we really appreciate the level of detail covering any hurdles to hopefully help us remove them for future contributors!

I am going to break each section in to a separate reply to hopefully help keep any further discussion easily focused on each topic. Each topic will have an Action to take section detailing my initial thoughts but this is all up for discussion.

One topic I would like to raise here is the general question of where any changes we make should live. We currently have the readme.md file that displays on the main repo page, there is a contributing.md which is linked from there and we even have a wiki with a couple of pages. I wonder if we should try to keep all information in one place (although still multiple files).

Perhaps we could reduce the amount of information in the readme.md file and have clear links out to other files like we do with the contributing.md file.

@brminnick @jfversluis @VladislavAntonyuk @pictos what are your thoughts here?

Action to take

• Find a suitable place to locate information

[TheCodeTraveler]

Thanks so much for taking the lead on this, Shaun + George!

These steps will help many future contributors be successful!!

[bijington]

GitHub

We certainly don't have any details on the how or the why here. I can see that GitHub do offer a set of docs pages here that we might be able to utilise for this:

• Contributing to projects
• Fork a repo

I don't see anything explicitly mentioning branching in there so this might be something we should aim to add ourselves or at least get in touch with someone at GitHub that may have thoughts on this topic.

As for the review process we can add some documentation here, I know full well it can be a daunting task and I am still not sure I know the answer on who should resolve conversations.

Action to take

• Investigate further possible information for branching, etc.
• Raise issue to cover adding this information wherever we decide in the Feedback section

[bijington]

Tooling/rules

Theres a couple of points here but I wanted to keep it all under the topic of Tooling/Rules.

I completely agree we want to strive for consistency in the code styling. To this point it sounds like we may have some .editorconfig rules that aren't strictly applied. For things like 'Block body constructors' or 'expression body for constructors' I am actually ok with either and happy to leave that down to the contributor. For other things like is null vs == null I would prefer that we are more forceful to meet our expectations. Of course I think this needs to be a fine balancing act as we really appreciate people wanting to contribute and we don't want to put them off with too much enforcement.

Code cleanup

I am surprised by this - reading the docs here they state that Code Cleanup should use the styles defined in .editorconfig. I suspect we may have missed some settings so hopefully this should be a relatively straightforward thing to fix based on the examples that you have supplied. I guess we may need to explicitly define more to cover the scenarios where developers may have changed from the defaults if thats what we are relying on.

StyleCop

I would prefer to see if we can improve the .editorconfig settings to rule out the issues that you have seen. This would mean that we only have to rely on one settings file and should make future rule changes easier to manage.

Action to take

• improve .editorconfig rules by declaring more explicit rules including defaults to cover scenarios where developers have customised their default settings

[GeorgeLeithead]

Improving the .editorconfig where possible, sounds like it would be the best option.

[bijington]

It is certainly the easier option unless we discover that there are some things we cannot enforce with it.

[bijington]

Discussions

This is a difficult one, having so many different components can make it tricky to work out where things should be raised but I think the following sounds like a pragmatic approach:

• If still under discussion (prior to approval and proposal created) then keep under the discussion
• Once a proposal has been raised but no work has been started then keep it in the proposal
• Once the work is in progress and a PR is open then I would raise the information there

If something is important/blocking then I think you want to add a comment to a file that blocks the PR from being approved, this helps a reviewer see that there is something to be actioned before the work is completed.

I will add that I do find it challenging to somehow track all conversations or comments on PRs in GitHub, especially where is a fair amount of traffic involved.

Action to take

• Not sure

[bijington]

Something extra to add might be that when the PR is created we somehow create a set of checkboxes - one per requirement. This might make it easier for both parties to see that all expectations of work have been completed. Of course we want to keep that list relatively small to keep the contribution and review workload also relatively small.

[GeorgeLeithead]

Need something comprehensive enough, but not onerous.

[bijington]

Using existing code as a reference

Pointing out that you grabbed the code from the codebase already is helpful, we are still discovering better ways of doing things as we go so there is always a good chance that we end up with some technical debt in terms of older ways of doing things.

It sounds like there was something missed in a conversation during the PR, I will continue to cover discussions in a specific reply - the AvatarView PR was an extremely busy one so I suspect it was easy to miss some level of detail.

Please do continue to raise points like this if they do crop up further. They don't necessarily need to be addressed in an existing PR as this adds a potential extra demand on the reviewer of the PR. I would suggest discuss it with the reviewer and if both parties agree to including it do so or one of the parties should raise an issue indicating what should be fixed in a future PR.

Action to take

• Not sure

[TheCodeTraveler]

If I understand correctly, George leveraged the .NET MAUI source code for AvatarView.

Perhaps we could recommend contributors leverage the CommunityToolkit.Maui source code instead?

We now have implementations for Converters, Behaviors and Handlers (eg DrawingView + Popup). Future contributors can leverage our existing implementations for their new feature contributions.

[GeorgeLeithead]

For things in the sample pages, I did use the other community toolkit pages for source and guidance. Popup for example. It used BoxView, but I was asked to change it to Line.

[bijington]

Release

Apologies here we probably should have checked with you that you were happy to bring the work to completion, I am guessing the assumption was that it was complete. We will make sure to confirm this with the contributor in future. That being said we are really quite keen to release small amounts and often so that we do not hold up developers wanting to grab new features. If we release with a known bug which isn't a showstopper then I think that could still be fine but we should be reviewing this.

Thank you for raising the issue and offering to investigate it.

Action to take

• Confirm with contributor that work is ready before merging and releasing.
• Or perhaps make it clearer in the documentation that what the rules/expectations are around a 'ready' PR

[bijington]

Monthly stand-up

There is a a link hidden away in the readme file here but we should definitely make this more obvious!

Action to take

• Link to the community stand-up (possibly investigate a video thumbnail) to make the link more obvious
• Add to the the location decided upon in Feedback section

[bijington]

Testing on all platforms

I believe this is the purpose of the platform specific checkboxes on a proposal issue. Perhaps we need to move some of this detail over to the PR template?

Action to take

• Discuss further

[TheCodeTraveler]

I wonder if there’s a way to enforce via GitHub’s PR “Checks”.

Right now, we require an Approval, all Conversations to be closed and all CI/CD steps to complete successfully.

Would it be possible to require a PR to force a maintainer to confirm testing before GitHub allows us to merge the PR?

Maybe we could implement something similar to how the license/cla bot works on our MicrosoftDocs/CommunityToolkit repo? It forces the contributor to take an action before the PR can be merged. Maybe we have a similar requirement that a maintainer needs to confirm that they’ve tested the code on each platform before a PR can be merged.

It’ll add a small step to our PR flow, but we’ll at least have confirmation that a maintainer has tested each platform.

[TheCodeTraveler]

Ultimately, DeviceTests should be the solution to this problem, but I believe we are still blocked on that initiative. @pictos can confirm.

[pictos]

Yes, we're still blocked on this one. Trying to make room on my backlog to move this forward

[GeorgeLeithead]

Are there any Kanban style tracking (Microsoft/GitHub) that could be used and linked to pull requests, and kept open and transparent? Might be a place to log questions, issues, tasks, relevant to such pull requests.

[TheCodeTraveler]

• Questions - We use GitHub Q&A Discussions for questions
• Issues - We use GitHub Issues to track bugs
• Tasks - Tasks should be included in a Issue / Proposal / PR

[TheCodeTraveler]

For New Feature Proposals, we use GitHub Projects

[GeorgeLeithead]

I really like the new use of project boards. Great work @brminnick 👍🏻

[CliffAgius]

Closed as per the Community Stand-up discussion here: https://youtu.be/N9wMcBP4jtg?t=2889