Hi good morning my name's going away from you us and you already have kind of many of you all about the quality so far I am going kind wearing both my half today so I have an open source maintain and was source contributors slice of that from both sides and high and I was a teenager like can I am also needs so it would seem that I am responsible for maintaining code quality with them as well so I'm gonna to talk about how to get your patch accepted this is predominantly aimed at people offering contributions all you know trying to do good work at work I have been completely unable to write this talk in a way that doesn't cause me to sort of trip over and start giving advice for the reviews as well as some point during the talk so we'll be looking at it from both sides of that relationship because I think ensuring that your patch is awesome is the same as ensuring that someone else's patches also was just an awful lot of crossover in there this talk is full of advice you never knew you wanted didn't ask me for and got some stories to tell but it all boils down to during ingredients that you need to get your patch accepted the 1st 1 is the code because the code is revision 1 of my project and I think the everybody pretty much does it the 2nd of the words that describe what this patch is or does it fixes maybe tells me what I care which can sometimes be a missing link and that brings me on to the 3rd ingredient is understanding it from the maintainers perspective all new team leads perspective what's the big picture where is this project going and on how does your change fit into that doesn't fit into that the 3 things that's very important that you have before you submit your patch bearing in mind that the 1st thing I will do when I see it is 1 what does this do you like to write the code and already on the inside of this process it just the rise and I go all 0 what's an iconic token it's and kind of try and understand what it is so it's really important that that information comes in and it's something that you think about as you prepare the patch about the current and then I'd like you to hesitate before you actually submit the hatch and cover a few more things give me some information the building of feature you fixing above what is this does it relate to a ticket now some projects will require the way the tickets and they like to have patches with commentary tickets a lot of the examples in this talk related gate and gate home because that is the way that I mostly work I think everything is transferable it's not a coincidence that the half issues and begin hollow-core requests all basically the same features like on a really connected In my project I don't require that you have the ticket if you say something that you want to 6 or enhance bands there isn't a ticket for an other thing you should have the paperwork to open the ticket before you can send me or some x this is 1 more hurdle in especially open source that's just annoying might lose things however the if you think code review process as a whole race so there's a serious offences annual horse may fall and 1 of these fences the Hollander criteria which can cause your patch to be rejected then you can think of it like that maybe not having a ticket is a problem I like to think of a college review more like a scoring system so if your patch is not related to a ticket I still really a problem that you don't get any points for that if a patch is related to a circuit perhaps you get plus 10 . it is related to more than once if you get minus 3 for each ticket is worth to interpret because when she I mean if there is 2 related tickets that kind of the same thing then fine but if you got to different things all fall completely unrelated things I did not want this patch and if you need some help preparing your branch correctly then let me know cool turned into full patches the nice thing about coming from a ticket is that we're taking it probably describes the original feature desired by somebody all the original blog as it was reported so if it does relate to a ticket you the contributor and made a reviewer we all have more information there might be some acceptance criteria something which says saw that the system but the that and instead it should do be if there are no acceptance criteria you need to invent so think about how this feature will be used whether it's an API and calling all actual user interface think about how it will be used think about what's acceptable and now trying apply those criteria to your patch because I'm going to I just saves time if you've done that already make sure you got

05:49

everything that you need all of the code and nothing that shouldn't be here so check that everything looks like it should it's really important to understand what's included and if you not terribly familiar with your source control system or you're not familiar with the workflow you might need some assistance or you might need to make sure that you know what you're doing so projects the help everybody you is responsible for reviewing patches has an interest in helping you make more awesome hatches never be afraid to ask trying to take you have included it which shouldn't be that the you can check out what's different between your clothes and last but remember this will include anything that's come on the muscle you main branch adaptive program since you started your right so instead try to div master with 3 and head shows you everything since the last common commit just the Committee on your current feature branch get to know your your due to did by itself will just show you that in line ASCII art the some constant to the we get to know your tools get data everywhere you have a command that is it then if you can replace it with gates diff 0 1 1 and that allows you to start to use clever tools that just bear in mind that green line there that the gives buy it at all you can configure your conflict your conflict well with this dot tool and say which toll you'd like to use and then uses so most of the time I used and this is my desktop told setting and that's awesome is side by side I can push changes from 1 to the other if things get really sticky sometimes I'll use which is a cross-platform and has a nice configuration settings you can get it open but rather than just pairs of files use over the whole thing you can see the training and look at where in the tree things change what the old and new versions of it is 1 that's getting to know your tools so that you can be certain about what you're submitting and as a reviewer uh so it's very easy to review other people's branches and understand what is coming in sometimes a patch can be too big there is such a thing as too much awesome incoming Carlos trust me at the back and this year I wasn't expecting it this is being really hard to review I had a dilemma recently well and I went missiles which is which is made of a few different open-source PHP projects and there's a weapons and maybe I can the API back end users different coding standards that print out of somebody offered me a patch which kind and mostly fixed 8 to use a different coding standard than the 1 of the 37 thousand lines of change I which I wasn't expecting an arrived on a day when I was on the right speaking at conference why can't maintain was with me at the same conference when I opened will request all of which would have to be replaced was about patch not really and it wasn't complete but we could we could have worked FIL dimension now I want to it was too much change giving me advice that a small patches and way more likely to be accepted as the bit like giving me advice about how to avoid the conflicts in source control systems candidate for the other person right but this is the same thing there's nothing to say you can't build a big feature that you need to build in such a way that it's digestable by whoever has to retrieve it and I was we have pretty regular release cycles as if we imagine that we just apply not quite continuous but it's potentially continuous we just click the X. button so by building a small change in a small change in a small change in a small change the features still gets shipped and usually faster than it would have it we have 1 big step coming years and we try to launch the last in common because of mistakes I see all we're trying and I have to hates relating to comment that more than 2 if you took colleagues let's talk about the 2 mainstream once I still it's not really exotic code with no explanation like if I have to go and look up the operator precedence rules in order to understand what your code does the problem we don't want my my code base that so we need to write code in the way that we want to read it at some future point the opposite is also

11:03

true so missing comment commenting because right source controls been around for a while but you can it but it has been invented and it means that we do not need to leave commented because In codebases it litters place it's hard to read and in illustrate this view is not marked common that common beginning of the line I basically can't say it was invented and I rejectable requests I was nonsense recently and it was because it was common to kind of clear to me don't leave things lying around your code if we needed later will just get it back how this works I was really belongs a genuinely happened to you last week our poor quest came in on part time in my day job as principal development so things kind of humor away for me to come into work and then try model of what number of full-time people do when I'm not there and so I popped in and that was a poet was waiting clan very urgent we considered lines which which along should be in in the morning my colleague had added a single variable called x some appears below the below acts as a single variable undocumented as an optional final parameter to we specialize in legacy system has not like he's messing up an otherwise beautiful code but really we can do that service no need to think to the lowest common denominator around you and we're under pressure pity brass and it wasn't the right thing and rejected the at the end of August a the lights and came back and I also mentioned were the missing weird but haven't really understood when the caucus came back you later on in the day it looked completely different there was no single character variable acts and that that was completely refactored away to do with the type of file that was something with an e-mail and you just referred to in a much much better way and so my code smell and my doubts was the right thing to do that 2nd attempt as is so often the case was released so much better than the 1st attempt and bravery on the part of the reviewer to call it and say that is not how we do that I'm going to talk about projects you'll find the file called contributing dock and the healthily get have doesn't show you this and we try to pull request the which point it seems to like because you probably need to know what your branch of being cold something so how look in the projects that you are contributing to because this will tell you do you need a it must the branch recalled something completely should it be re basal squashed or any of those of it's also worth checking indirectly with the project maintainers most of them have chat channels all mailing lists although the conversation going on the issues they feel and all that is not real time people and that is worth talking to them just taken attempt to let me know you working on now with the help of projects something that is emitted many time can you right your cut try as I can I can break there was a 1 contributor laughing at me from halfway back in the real life I can always break your code I think might and my developed as a work people I work with think I have some kind of magical mystical power because code to just malfunction spontaneously as soon as I get my hands on it and the truth of the matter is this isn't the magic and it's not even rocket science is not hard I do things like if the data missing or is it overlapping or is it a little bit ambiguous because you've allowed me to edited to many different ways and I have deliberately tried to convince the system all and it's all sometimes log in as a different user that seems to throw everybody to many developers building beings organism in user and friendly or not logged in as having is all those things are easy to try the truth of it if the user is an idiot and I have extensive experience of writing code that was broken by media users and so now I do a broom good impression of an immediate user the reason that I can break your code is because my colleague has been broken so many times before so trust see if you can beat me to it has a pull request the icon break so impresses me the I remember being on site with the media company in London this select and quite an angry he lay person showed up at that point that is never going to be quite an angry QA person work and she said I very angry I cannot Rachel Carson she said she as it is and you all to ferry about and pull apart code and find the blogs and in my college couldn't find the bugs and she said but I will she went but maybe it's a I think that's a compliment because I have always tried to think about what could go wrong and that's something which will enormously help most patch accepted if your contributing to a project but just to keep things moving a work to get features ship if you're thinking about these things if there are automated ways to break your code maybe there tests that you can run coding standards checks which if failed will cause your patch to be rejected and joined and has a syntax check and ever since the time that occur maintain and all I both were running develop articles on different there's of page into the life blood and gas what they committed a change and I reviewed there's change in betweens brought up a welder so now I have a syntax check that runs on bills so did how has this commit API at its status API you open a pull request and it will run the bills for you as calls out Jenkins interest Travis really really helpful this is fabulous because it means that bad news like your code might be kind but you'll in your in contravention of operating standards and therefore we will accept patch that news gets delivered nice and early have like the syntax check that's worth just having them in half the time it's complete market somehow made into a file 1 thing to really look out for and it's something that I think is developers we occasionally underestimate the importance of and that is your MIT

18:13

messages the Committee messages that you write on your journey to hack around to end up with the world's most perfect patch do not answer I do see those I do read the logs of my projects i call it archeology right so wecan legacy systems so things arrive and I did about to understand how all the we ended that where we are today and these matters this does matter if you get to the end of a branch and into the math and you don't know what to do figure it out this I use a lot so all it does is takes everything in your branch Senshi branched off master and makes it up on stage change so now you can cure right me some new commit from all that change that you just put another branch either it was a single committee and I've complained all it was a bit of a long journey of commits that when the bit brown houses sometimes we can't tell at the start of the feature quite how that's gonna look at when we finished if it was all very straightforward we knew exactly what to do but I actually others will be doing X would be bored we only the challenge in the entertainment so sometimes can be and very scenic journey to eventually arrive at the working solution so please 1 along with the topic of commitments is used I need more information than I get into this piece that might be a 1 liner that describes what he what they said I was queries a lively and soon to be talented junior developer here Our work for use as such we get yes sorry that's fine you don't besides the story outside the story you that I will also my my slides I would I I like as a soon-to-be talented junior developer and he put through a commit message are we commit to get lambda lab calls the few book if you book talks with flat channel give and the commit message said shuffling the job script files around which caused me to simultaneously saying do you call that to commit message and also pop open the data to see what he had done yeah they weren't very much shuffled John that follows around by need in the commit message I need more information than I get from just the data the tell me something about what I never really did find out why we had shuffled jobs propose around and if we ever have a problem with when we the day that we remain all the jobs follows among commit I How will still have no idea what that happens and that's why this matters don't tell you what's in the days I'm gonna read the diff anyway every committee has be parent that day who did it when I did it commit message use the missing magic it tells you potential why something within the context of this chair and if you just write fix all of the things and I'm gonna hate worse than that commit messages by by other people but sometimes you want the stranger you come back to code in 3 months nukes left you are the stranger reading your own commit messages and painting yourself do yourself a favor symbolic yeah there's not of and of a really helpful notices as this seems like a really strange change but there's the logic and explain the logic these are inaccurate commit message ICT many which are describe the most recent commit and kind of forget what they did in the branch before again and a lot of information sources and people could be chipping away doing a bit more aware a bit more work on feature I need all the these Chinese some kind of staying home I have a favorite list of things should do for Saint Paul Martin it's here this article is so good that you just say just up 20 minutes around most with the article to summarize a headline that isn't very long which is gonna fit on get lowered with 1 line is going to be is gonna fit in the chat notifications right headline if you need to write more words blank line will work and write as many words as you like give examples of command years give bullet points the the links give anything you want to i am by the famous or infamous depending on your perspective full writing commit messages which are longer than the day if they relate to I didn't know this was a bad thing sometimes the change can be very elegant but the reason and the journey together there can be a little longer than that I don't be afraid to write it all down please keep your commit messages safer work it doesn't matter How hipster all hobbyists local out there and you think your library is however I am a consumer of your open source library I build things on what you've made and sometimes I wanna do that in a commercial setting I don't want to read your gender-biased homophobic racist comments it's unnecessary even really just well although process justified so please say so what there's 1 thing you can do in a commit message which will automatically get your patch rejected if I see an apology right I donated I'm not taking it and the reason is it is good enough there is no need to apologize and it is not good enough while there is some good enough and maybe we can get together and try again but if you apologize and not just commitment is a citizen goes well not it not only that I'm so sorry about the state of this patch I'm so sorry this is the best I could do any kind of sorry makes me go I probably don't want this commitment so this is the right this is a real what was the kernel of a code smell commit message now like what has already given were does not when you in a patch it's worth checking if it will merge and it wasn't somebody will probably ask you to do something about it talk has like no green button scenario but page that whatever the validity of the green button but anyway there isn't 1 because it's going to conflict so it's worth checking if it will merge they can check by just over the course you can also check by seeing how that emerge will look so you can switch over to the master branch and do this with no committed no effect so 1 can call that commit the 1 complete emerge but you can see it is going to be conflict will tell you and if not the answer you can you can use this set basis something like this to do a dry run on whether your branch will merge into 1 of the target French it's it does emerge on and you get most of such a book to get your repo out of emerging status back to being normal it's as if nothing happened at this point and emerge then fix it and that might be there's a couple of ways you can do that some projects are happy if you just measure the master branch into your branch idyllic conflicts and be more textbook and occasionally required way is to read based you'll features so if you've worked on speech on a branch things that happen a master which would have caused conflict spells they you should read basis to take your branch and we apply only to the master or develop or whatever your main branch is if you already because there are conflicts when you merge there will be conflicts when you read rebates like there is the conflict between these 2 branches and you're gonna see it somewhere along your rebasing injured have seen some situations where there's actually a lot of conflicts along the rebates journey because your applying can be applied in a lot of commits so sometimes and then merge the merge is easier because it avoids all the crossing over the line happened in the middle of the ranch said replacing opening up call request on the is not just the action of throwing code over the hedge done Figure it's the beginning of getting your patch accepted we able

27:32

request cheque the source and the target of correct and this is surprisingly easy to get wrong and I still do it sometimes the source will be your branch in your working as an open-source contributor that's probably on your fork you share in repository commercial project that's very likely overshadow repository an entire branch will be I Marcelo developed or whatever the talk has a setting full what the main line of development branch is so usually default correctly but it's worth check and then scroll down and have a look at what did that's generated this will tell you feel very other islands of the difference like nothing you've ever seen before something is terribly wrong and and you should just like close this web browser window and try again check push the breadth it's called the right thing I don't know something is terribly terribly wrong this is usually the moment which you realize that you branch from completely the wrong place for something else terrible has happened people request means that title most people will be released to pull requests and so I will only see the title broadcast have to be open quite a long time on the list before you get to know them by their ticket number so you could possibly include any extra information maybe some words to go to go with that ticket number that would be awesome because all know what thing is on the listener might remember why not fair enough so you know I have 45 minutes going next phone call I'm looking at least a poor requests and the 1 that says copy change on the navigation will many offered at sentiments that are enormous says to give 42 you realize that numbers and it's right up there with submitting small patches is it it's about perspective is about remembering the maintainers and the years I've been saying the sympathetic to open source maintains that busy people that volunteers but actually it's not much better work because I'm very busy that I'm sure that's true for everybody as not special really I always try and lower the acceptance barrier and that's how you get your patch over description I need some words I really some words I had 1 goal request opened and I I think it had a single committee and the title was the same as the commit message which was implements the get cash function this is a bit like implements the dancing bad feature was a really low dancing bear with me maintain that this again cash function and and thinking well I come from a reduced from retirement and actually it's not even interested contribute to this conference came somewhat definitely should have and that so and in the data this and you get cash function and inside get cash function there's code that looks like cash code happen but nothing at but it's not called from anywhere at I have literally got a committee that implements get cash function Johnny could use the this does not help write me some words what actually happened was we had somehow eliminated the get hash function from the code base don't know how many people that was the reason that the search function was broken across all platforms and all we needed to do was employed get has function which is what he had done but I am not a huge hairy and maple request which he did not did not allow me to understand the urgency of the situation was I had no idea why was fixing so it tell me what it does tell me what is the exactly how to test it on what we changing about what you would find the moment likes paperwork that ISO 27 and other 1 compatible and they like lots and lots and lots of work is very interesting because it means every feature we build or every but we fix or every system change that we make we have to write how to test that it isn't there and then have test that is that and actually that really makes me think about the big picture what the users will say and how that would and it's quite that's quite good information but that would be really helpful to be included when you're submitting tell me what I can however it is my job to care probably measure change eventually like the crime was out of will get in open source that's not true as a lot of abandoned I'm sure all of you of vitamins contributions still offered contributions and some of them have been accepted summarizing their response good question why is there in you know answer tell me what I can do you want to get my attention you want me to invest my time in bringing your code into my projects so that I can be responsible for it forever please tell me what I can this ribosome words review and is it's a process and it it changes a bit depending on what exactly reviewing what the context is but not all of I've been an open-source maintain now for a number of years and I was contributed before that and so contribute to a bunch of other projects going was followed by rules so probably not but I think there are some things that we can do to prepare suppose review to make sure that our changes are already and as reviewers this is the point at which I could avoid tripping over becoming code review because sometimes that's my perspective take and I'm sure that all of you will and perhaps should review as much code as you can to be a good writer you need to really need you really well red by the that's true development 2 you need to read other people's code and to actively participate encodes review and in evaluating kind of think isn't really not like to do that when you're looking at of for the 1st time with title and description and think about what shape you would expect hatched to the the 1 come intraspecific ticket and tickets clients across the copy change because of you today different that they had quite complicated SQL statement and and when was but I didn't system very well went over a long have didn't system very well it turns out they have the CNN so in order to make this copy change across all the platforms when he to do that by adjusting the records that we call stored for them in their CMS system and we were just doing it had dimension and doing legacy system yeah we just doing

34:59

that by entering a light into that space because it dynamically generates but because I want who is nothing wrong with the SQL statement I mean if you just read the coveted code that's fine as a code review and you need to do more than that we think about is this the right perfectly correct these accountant or is this just a perfectly correct this approach does not bear any resemblance to solving the problem in front of us at but grammatical point please don't merge someone's pull request please review it I want you to keep that attitude of skepticism right until you can't prove that it shouldn't be it shouldn't be accepted this is really important come to our computers and these emerged as thing and also time I don't what it is is not ready or it's not done or I can break all of those I have a good 1 recently where the same junior develop as a result of but at the same dinner developers and on my and this merits emerge request but is also reasons so I will axons like good man so you can magic president took him flies attempts to get me to merge this page is 1st of all in the database fast enough right numbering nearly and then about something else and didn't like this piece of metal type and all my goodness he's the old a diagram of this thing all of these things he could have picked off before a and hopefully that will happen next time I feel like you learn something but just keep keep that cynicism a little bit longer and reviewing be an to it but don't try to merge it when I try to merge code I accept things and then it probably gets the test problem and I realize it's not complete I think anyone can be a kind review I don't think this is the responsibility of the teenage all the maintainers exclusively at work whilst operating system of peer review so I don't mind who writes the code and don't mind who manages the code as long as those are not 1 and the same which means that the most junior person in the company also reviews everybody else's code and I review here is and we all kind of share ideas so he sees my changes and often pixel bugs in them and that's really important to he sees how I would approach a problem in open source then this gene can't be done by just anybody because we tend not to just opened up right access to a repository but having input from other people on an open call request makes me when I likely always more confident that this is actually correct I'm mostly work on joint in which is an open-source I've been feedback site survived the conference organizer running a local that copy pulling of features and this looks great all I'm not sure with that really influences whether or not I mutagenic set we had a board last year unfortunately I had flown samples it's going even more importantly with my can maintain this is a mistake the body's advice do not travel with OK maintains I once had the other the other can maintain and the only other person with server access all of same transatlantic flights also gone with me as legislative the chances of anything right and need to wait 24 hours and I more on this flight so sorry because the flight number of internal happens I mean anyway as goes and I woke up at 5 AM local to e-mails Twitter stream everything about some users being unable to log in on joined the so what I was sharing an apartment and I'm very optimistically logged in velocity and he was not obviously weight because he was not in those like OK so now I know who is fixing this thing however due to the time zone differences 1 of our contributors in europe had written a patch and so I looked at each other was like I see a problem I see a poor quest where we are with this and I have the ability to put anybody's branch onto the test systems and the back some people tested that someone else hold on his death couple and tried that because people were able to make it work on someone else's they're and on the test platform I pressed the big green button and get help which can be used because I can't I don't believe that code review can be done other than like on everything without calling the code looking at code running the code cooking the cut by I don't have a good relationship with this button on a web interface so I don't think that's how this works on this occasion I click the green button there click click the go button on Jenkins to the but His somebody else had done that code review and if you want a feature accepted to come back and say hey I really liked it went me on this platform you know the other I am still learning learning I need to check the the pull request on the project before it and then the poor quest is half the time being I'm going to fix is already been fixed but sometimes it's still sitting in a poor question if that happens Poland branch while means but test and feedback on a more aggressive that had tried it really looks awesome to me on this vision on my couple MACsetra there's a reason that says you can't get involved the merging will actually be done in different ways for different kinds of projects and I like the peer-review I enjoy getting the whole project involved by its very nature an open source projects we have the gatekeepers we have people who take that responsibility and I work is something right so I will say to manage and might developers of stop saying to me on what was like the balloon to just realized I think that's a responsibility thing we take it quite seriously as a kind of you know there are some key skills that you need and in particular the ability to see what are not looking at is absolutely me it's very easy to find all the and maybe the data looks good but it is the things that are not in the data which causes you all the problems and this is what makes you I will also but what will really get your patch accepted already enable you to get a complete hatch and make that work does

42:26

this mean documentation this is my number 1 reason the declaring work all requests which a something fundamental and no documentation go with a close only to which later no part the patch that is not in close maybe a hot but it is improving our quality and those legacy projects that reading the times the required tests do think this feature could new detector I will sometimes not close but do that a poor quest we like this will be acceptable when it also has the protection on anything database touches the somebody make a local database change that we need to make this kind work and is not in the past they because it's a new file maybe I forgot to add it that happens deployability I'm not sure this is a word but it's a really important issue is the ability to run this code somewhere other than on your on your possible obtained works for me is not how we ship software well what's could really good starting point but works for me doesn't do the rest of it has to run on your desktop on but then on my desk and the test that will life all you have to be able to see the difference in which came in which change the config and it's like you just raise a path with no respect for what platform your on this curve with just inherited only runs on 1 platform the life platform to not only when someone comes from the dead platform that's going to be a problem when I deployed looking out for those things that are hard to the because they're not that the forgotten changes to 10 plates the following changes to e-mails when you remember to change the templates reporting systems so I have I have a client who I think we must burden there was 3 times the last 6 months because neither they nor weaker remember that we have reports and so we do things like beautiful amazingly factors and then we update their e-mails and all but 10 plays a website it's great and then reporting breaks at the end good enough to see that this change is not in your day monitoring changes this is sometimes invisible to developers and I'm in the middle of doing a big refactor of giving workers because running like 20 different ones and and going to group them up so that it can be more than 1 things all have like 5 different ones will have 20 of 1 of them this is going to break the monitoring system completely which checks that certain dinner jobs running time so more things to bear in mind from jobs and can be really hard to remember especially if the conflict for better or for anything is not in your reader these can be really difficult things to remember as a reviewer issues this difficult to identify and then can be controversial to deal with if there's something missing and this kind of sick people particularly if you want you develop reviewing for the senior developer she doesn't want her patches rejected these are not good enough and it's the right thing so don't be afraid to reject it or at least to push it back and say I also want I think this would make it completes we should also include the following push it back and if a change comes in like my decoding some things that you don't want you know you're never going to merge dancing bear feature because you don't want closing rejected it's kind it to everybody just just and we can iterate or not but we have to be front and we have to be able to communicate about the sometimes the rejection will come automatically because you're running those automated builds a fellow code itself if you're not accepting a patch really important to include constructive the back I had 1 situation where a new employee and had you know he was really nuisance was that the guards at the 1st time in fact also time office right see it quite a long quite longer I would just everything being hardware after where it is guy right so this is fabulous this is now much better Italy things well 1 day I think merge in commented that it perfectly good progress dilemmas and I respond my colonization was like it was called by really didn't like the comment syntax that you use the the comment syntax is valid in PHP we typically used his last you can say that the internal and that so much that was also and yeah because all you can say it was wrong the comments intact yeah he was right on side I did not say well done you'll pull request is otherwise perfect and you have worked hard at this I said it a problem that that's not very constructive and it didn't recognize the work that he had done the the that gave does depend on little bit on the retail for an open source contribution people have taken the time we probably don't know them the volunteers and the between iterations can be quite long I am more likely to add 1 more committee onto the branch to fix whether is missing or with a small thing and then merge it myself with colleagues I have to work with these people and I don't want to be picking up after them all the time right so they are very much more like there was a paid to be aware when I was so that there is much more likely to get no do where is an open source unless I know so joined in has a lot of new contributors has a lot of first-time open-source contributors if I know those people involved because they want to experience may want something better than will be like that and they can improve otherwise all common why did and fix it myself emerges so my approach is quite pragmatic and I'll do often some combination of there is such a thing as too much feedback please I would never ever have mentioned the thing about I don't quite like your commenting syntax if there had been anything else for all without much if there had been anything where I actually couldn't accept it I mention all of those things but nothing something which is a pretty good 1 thing to change and in general and not thought of ternary operators think hard to read that probably would say 1 thing at all for the extra feedback otherwise I'll just fix it as it comes from so it's about finding the appropriate feedback gets the patch accepted but that also gives that contribute to all that code colleague what they need to work with you I I spend a lot of time Toastmasters which is a public speaking organization helped you to improve your skills the give a talk and then you get some feedback on the it evaluates and maintains an approach called some which technique but you start with something positive the filling of the sound which is something that you can improve and you go on to end with more bread on a high note and I think this is something that we can do in open because we don't do very well saying thank you and we could start with kinds for offering me all requests thanks for taking the time for this quite like it actually worked and the then looking forward to seeing more code I don't know as much there's also something called the modified some which which again starts really positive mentions what isn't working and then gives constructive advice on how to change that you commit messages all 4 please read that link that I mentioned earlier so thinking about the feedback that we get sometimes you have to deal with harsh feedback this will happen more likely a lot of people don't offer patches because they think they're going to get blamed it's actually massively more likely that you will get the thing itself this is the whole of abandoned bullet rests on the internet if you get a harsh feedback it is personal the by code some value but also then the rapid somewhere else open source has a place where you are wanted as a place where you are appreciated has a place where you belong and if you haven't found it in your open-source journey will continue until you find the projects or an organization or group or anything that you can contribute to which don't make you feel victimized how much feedback the official advisers always grow thicker skin and I don't that's find a place where you feel comfortable offering of changes and interacting with will require I'm not the throwing of code of the hedge is done here you guys it's the beginning of the conversation they have a lifecycle you may need to iterate other people may contribute before this feature is finished but by starting the conversation you become a contributor the Apache accepted you

52:22

only need to remember the 3 main ingredients that your code correct it completes test it make sure it's ready right some words to go with it explain to me why I I can remember my perspective maintain always development my colleague of their very much getting to know the things I am going to reject without even really looking up and get to know that look at the right the project thinking that it's with these 3 things you will get your patch accepted if here we have about 1 7 minutes for questions 1st thank you for most was eventually and yes that's a question comes going to willing so used to being 1 and so I appreciate the scenery of the various services you need feedback about rejecting patches if there's a there's an apology in the commit message I'm wondering how you balance that out with first-time contributors may have impostor syndrome were going to apologize for reading little on for the quality of the patch and what eventually get people in that situation and the rest of the time you see a lot of this kind of some wasn't such a close edges mock then in a really friendly way I try and let them know that they don't need to apologize similar similar that that the message and just try make it a bit more factual around what's actually happening because there a problem and but it's yeah it's still trying to take it on its merits my colleagues however you will will will get committee will get that has rejected with a commitment is that apologizes about I think you forgot to some think most compartment in the code is checking light that you don't use light systems that will violate licenses and so on so that the developed part has to check life sentence before use libraries that allow the you hold it in his history of documentation is must tell but she also said that you don't want comments and the post so where do you want to have the documentation I do 1 common because I don't want commented code don't want come that is an operational that's commented out I do want do always 1 comments and I normally like to have additional documentation might API documentation for example user documentation that would also need updating so that needs to be considered a changing of feature an enzyme 2 more points and what you see in outside the US is a lot of code with commands comments and WA remains of different languages so opens and OpenOffice spend years in changing on German and Russian comments into English using that same partons use just 1 language and those him he's also indicated that you know that the next thing we have to stick into the net and and and it's very important for the project it settles that they have the Vanni a restricted coding guidelines coding style guidelines I from was just kind of you get a patch features to 2 and the layout of code not OK so and I think it's very important that you will lead lead told well the top not has phosphate has anybody I must I actually have stated that the eyes of the whole estimated that you submit people request the builds run it it's not comply with the style guide it just reject that so and and that's 1 last piece about me is that I don't accept that I myself and I have always lived and because it is not my slide my brain is so empty right now I would never remember thinking appears came into my own up 57 per cent of it up and the me is really early that got the it's he has comments here in this room just mentioned and that's the finding that only through not thinking at the moment anyway and like and I think it's a good thing for the club like this because a lot of people don't know by itself is rejected and halted its improved for thinking for this problem thank you yes in these different modes it

Inhaltliche Metadaten

Open source is built on volunteer hours. Using best practice in creating and reviewing patches means we're making the most of those volunteer hours - both from contributors but also from those overworked maintainers. Lorna Mitchell