I recently had my first PR (Pull Request) accepted into the main Xamarin Forms master branch. It wasn’t a big change, just bringing WinRT/UWP into alignment with iOS and Android in regards to center cropping images when in AspectFill. The whole process is quite interesting and convoluted that I thought I would document it for others.
The first step is to fork the code, understand the code and see if the changes you want to make are feasible. Go to Xamarin.Forms on github and Fork it.
Before you even start coding you need to ensure you adhere to the coding guidelines. Xamarin follows the .NET Foundation Style, with 2 exceptions below.
- We do not use the
privatekeyword as it is the default accessibility level in C#.
- We use hard tabs over spaces. You can change this setting in VS 2015 via
Tools > Optionsand navigating to
Text Editor > C#and selecting the “Keep tabs” radio option.
You must follow these guidelines or your PR will not be accepted.
Forms-devel Mailing List
Before you even start writing code, unless its a small bug fix, it is best to subscribe to the forms-devel mailing list and discuss your changes.
To subscribe or unsubscribe via the World Wide Web, visit http://lists.ximian.com/mailman/listinfo/forms-devel or, via email, send a message with subject or body 'help' to firstname.lastname@example.org
Then to send something to the group use the following email address. Make sure you include a descriptive subject line as this is used as the focal point of discussion.
Send forms-devel mailing list submissions to email@example.com
A Xamarin team member will normally give you a go ahead to submit a PR to make a change.
Exception: If you have submitted a bug to bugzilla and it doesn’t require any ABI or behavior changes, then you are unlikely to gain benefit from the mailing list and should just submit a PR.
Update 2017-02-12: While the mailing list is still there, it has kind of being abandoned. If you want to discuss a new feature change, it is now best to do it in the Evolution sub-forum.
If your suggested change requires an ABI break then you will most likely be asked to take it through Evolution. This is a process where you put up a proposal in GitHub on what you want to change and why. Xamarin team members and possibly other developers will then discuss it. If approved then you would start coding for the PR.
Update: Use the evolution sub forum now, not this. The github page never really took off.
Now you can get coding and implement your change in your forked version. If its a significant change make sure you write unit tests. For small changes you generally don’t need to write unit tests as they are covered by existing unit tests.
Xamarin Forms also seems to lack unit tests for platform specific code. I am not sure why, they only seem to unit test the core project.
Before you submit your PR you also need to rebase and then squash any superfluous commits as to not pollute the master branch when it gets merged.
Rebase with Origin/Xamarin
# You only need to do this once, to add the keyword Xamarin as the remote origin. git remote add xamarin https://github.com/
xamarin/Xamarin.Forms.git # This gets the latest from Xamarin git fetch xamarin # Rebase git rebase -i master
If you have created any new public API’s, you will need to update the docs. For Windows, in the solution folder run:
Failure to do this, will result in automated build checks failing.
Squash Commits (if applicable)
That last rebase command will show you something similar below that you can then
pick 9dfce34 Commit message one pick 348d572 Commit message two # Rebase 9dfce34..348d572 onto 8ffe67a # # Commands: # p, pick = use commit # r, reword = use commit, but edit the commit message # e, edit = use commit, but stop for amending # s, squash = use commit, but meld into previous commit # f, fixup = like "squash", but discard this commit's log message # x, exec = run command (the rest of the line) using shell # # If you remove a line here THAT COMMIT WILL BE LOST. # However, if you remove everything, the rebase will be aborted. #
Change that second pick to squash to squash that commit to the previous one. This is just a file you are editing.
To save the file, press ESC, then write
Finally lets push this back to your fork.
# Rebase git rebase xamarin/master # Push git push -f
Now head on over to your fork on GitHub and create a New pull request.
It will check if there are any conflicts, which there shouldn’t be if you just rebased. You now have to fill out the description according to their template. This is mine below.
### Description of Change ### Images on WinRT and UWP will be center cropped when in AspectFill to provide the same experience that Android and iOS currently provide. Change was discussed in forms-devel Digest, Vol 1, Issue 9. ### Bugs Fixed ### ### API Changes ### None ### Behavioral Changes ### Images on WinRT and UWP will be center cropped when in AspectFill. ### PR Checklist ### - [ ] Has tests (if omitted, state reason in description) - [X] Rebased on top of master at time of PR - [X] Changes adhere to coding standard - [X] Consolidate commits as makes sense
Then submit once you have filled everything out.
CLA Requirement Check
The DotNet Foundation has a bot that then instantly comes and checks your code. If it is significant, it requires that you sign a CLA (Contribution License Agreement). If its not significant then it will comment that one is not required.
Now we are on to a real developer review.
Developer Review and Checks
Now it goes to developer discussion. Xamarin team members will review and thumbs up or down the PR. What looks like a 2 developer thumbs up, they will then run it past the unit tests to ensure it passes.
I don’t see any consistency in times when they get merged, but eventually it gets merged into the master branch, then will be tied in with an official release that will eventually end up on Nuget and available for all Xamarin.Forms developers. Mine took a few days from acceptance to merged.
Rinse, wash, repeat 🙂
(Please let me know if there are processes I am missing or inaccurate information and I will update as appropriate)