246

This happened to me last year while I was interviewing with a company for a position I didn't get. I'm currently employed elsewhere but I'd like to know for future applications.

I had an excellent phone screening, according to them - they said I was a strong candidate, and the first technical interview with one engineer went very well. Between that second interview and the final interview I found their software had an open-source API on GitHub, written in Python. I looked at it for a while and found a much simpler and future-proof way to write one function, and I opened a PR with the change without mentioning that I was currently interviewing.

When we started my third interview with two engineers one of them mentioned that he saw my pull request and it was inappropriate for me to open it. He said that it came across like I know more than them as a fresh college grad, and that I haven't considered why they coded it how it was. I didn't end up getting the job.

Was it really inappropriate for me to do this?

2
  • 85
    What precisely did your comments along with the pull request say? They might have taken offense not to the pull request itself or the code therein, but to what you said about it.
    – user1602
    Commented Mar 6, 2019 at 8:57
  • 2
    Comments are not for extended discussion; this conversation has been moved to chat.
    – Lilienthal
    Commented Mar 10, 2019 at 10:55

13 Answers 13

286

The part that gives me the most pause here is "a much simpler and future-proof way to write one function." I haven't seen your code and have no understanding of the context of your change, but it doesn't sound like you fixed a bug, added a feature, improved performance, or otherwise did something that the project maintainers considered meaningful. I can see how submitting a pull request for an unsolicited change of this nature might not have left the best impression.

Many open source projects (and frequently closed source development projects too) aren't run like Wikipedia articles where everyone is encouraged to make small changes all the time. There's a non-zero cost that comes with any change: the time to review and test and approve it, the risk of breaking something (even with a robust test suite), creating something that fewer team members understand, etc... As a result, many projects aren't particularly big on changing things just because; there are an infinite number of ways to write any function, and nothing would get done if everyone regularly changed existing working code to meet their personal definition of "best" without justification. These are norms, and like all norms, they vary and are generally things you're expected to pick up through osmosis rather than be taught. If you were a recent graduate, it's likely that none of this was particularly apparent at the time.

Most pull requests address a more obvious need like fixing a bug, adding a feature, or improving documentation or testing, rather than changing a function to accomplish the same result in a different way if the maintainer doesn't see value in the change.

So I don't think it's inherently inappropriate to ever submit a pull request during the interview process (it certainly shows interest and enthusiasm), but from their perspective, they may have seen you as someone who is likely to rewrite working code without much justification, and then they, unfortunately, reacted negatively and condescendingly toward you. Which, helpfully, tells you a lot about them and what it would have been like to work with them.

Edit: this is several years later, but as this question continues to attract views and votes, I feel like I should clarify my point a bit. The concern the interviewer may have felt, especially if the benefits of your PR weren't apparent to them (one person's "simpler and future-proof" function is another person's "why did you rewrite my working code for no reason?"), was that it could have appeared like you were browsing the codebase looking for something to change arbitrarily for the sake of making a change. There was still no reason for the interviewer to react so condescendingly to a PR, but if it felt to the interviewer—even though I'm sure this was not your intent—like your change was submitted more to boost your prospects as a job applicant rather than to materially improve the project, it may not have left a positive impression.

17
  • 43
    I agree but the part "He said that it came across like I know more than them as a fresh college grad" makes me think they did not have such a logical reason for reacting this way.
    – Qwertie
    Commented Mar 7, 2019 at 1:10
  • 171
    Agree with the conclusion, but as a maintainer of open source projects, I disagree with the rest of your answer. I value every contribution, no matter how small, as long as it's an improvement, is justified and displays understanding of the project as a whole. Even if it's not, I can't think of a single reason to react negatively - even when the code is objectively bad, you just thank the contributor and let them know you won't be merging their code. Reacting negatively towards a person who spent their time and made a genuine effort to improve your code is a sign of poor company culture.
    – Egor
    Commented Mar 7, 2019 at 3:30
  • 80
    @Egor +1: It's a pull request, good heavens. An offer. (And besides, it shows enthusiam and, hopefully, skill. I'd react negatively only if it didn't.) Commented Mar 7, 2019 at 14:56
  • 19
    @this.lau_ but at each step they can reject it. They don't even have to review it if it causes that much pain.
    – user87779
    Commented Mar 7, 2019 at 17:44
  • 54
    @user87779 I'd even say they shouldn't have open sourced it in the first place if they feel like accepting pull requests is a burden.
    – Egor
    Commented Mar 8, 2019 at 0:12
439

Clearly it wasn't a good tactical choice for this company, but it's pretty silly to go to the trouble of setting up a public repository and then disdain people for having the chutzpah to submit pull requests. Saying "No" to a pull request is hardly a burden. Heck, they could simply have ignored it.

If I'd been the interviewer, I would have given you bonus points for demonstrating real interest in the company and showing that you know how to work with an open source project in a public repository. That would be true even if the submitted code was naive about the problem being solved.

Since a job offer is on the line you should be sure that the code you submit is of high quality (get somebody else to look it over), and keep any comments in the code or in the pull request humble and polite.

13
  • 351
    Remember, when a prospective employer is evaluating you, you should also be evaluating that prospective employer. You have successfully avoided working with a supposedly senior developer who doesn't even know what a public repository is for. Commented Mar 6, 2019 at 5:15
  • 46
    Not to mention I would treat such repository exactly as an "extra points" stage where you can show a)you are interested in the company b) you can read the code c)you can evaluate it d)you can make it better. Commented Mar 6, 2019 at 9:30
  • 10
    "keep any comments in the code or in the pull request humble and polite." add Professional, and that sentence is a good rule to always work by.
    – PeteCon
    Commented Mar 6, 2019 at 14:30
  • 8
    @A.I.Breveleri - the fact that a repository is public does not mean that it is intended to accept unsolicited contributions. Many github repos especially from companies are effectively publish-only, others are convenience mirrors of projects where the actual development flow and change submission occurs elsewhere - and since github does not have any feature to disable pull requests the fact that one can submit one there implies nothing about the repository's purpose or the project's policies. Commented Mar 6, 2019 at 16:05
  • 42
    @ChrisStratton but the same logic applies to the submitter too. Unless there is some dire warning printed in bold in the README for the project, or in a contribution policy, submitting a pull request may be pointless but it's hardly inappropriate. Commented Mar 6, 2019 at 17:24
107

You have misunderstood the feedback you were given and focused on the wrong part

He said that it came across like I know more than them as a fresh college grad, and that I haven't considered why they coded it how it was.

The problem isn't that you issued a pull request, but that you did it for something that made yourself appear arrogant, ignorant, and unaware of your own limitations. You describe your change as making their code "much simpler and future-proof"; it seems obvious from the quoted section above that they disagree. What's more, since they are more experienced than you and familiar with their own codebase it is very likely that they are correct and you are wrong.

It is common for graduates to emerge from their degrees strongly over-estimating their own competence. They weren't annoyed because you issued a pull request, but because you appeared to demonstrate a lack of understanding of your own limits and a lack of respect for their skills in the submission that you made. Likely, you compounded this impression during interview.

Finally, don't read too much into any particular part of any particular interview: it may be that this had nothing to do with you not getting the job and they just had a better candidate. You don't know. Don't obsess over this setback and instead focus on the next application. Good luck with your job search!

14
  • 13
    I agree that probably the issue is less the pull request in itself but its perceived quality. The company's perception may be wrong, in which case the OP wouldn't want to work there anyway; ot it may be correct, in which case they wouldn't want the OP to work there anyway. Commented Mar 7, 2019 at 15:01
  • 8
    Your comment would make sense if it was a private repo
    – user86742
    Commented Mar 7, 2019 at 16:40
  • 5
    @RobertoTorres Jack is not discussing about the PR itself, but how was presented: by quoting Jack answer "something that made yourself appear arrogant, ignorant, and unaware of your own limitations", basically someone possibly toxic they dont want in the team.
    – Lesto
    Commented Mar 7, 2019 at 17:15
  • 4
    @Lesto cause they'll find a junior who isn't a drain on resource and who doesn't need stuff explained to them.
    – user53651
    Commented Mar 7, 2019 at 23:11
  • 3
    @Leonidas personally I prefer a mid-quality developer that integrate good with the team than one top-tier but "difficult" developer; the moral hit in the company could destroy productivity way more than that what that dev brings in. Of course we don't have enough information to say this (or the other way around!), but this is a realistic possibility for refusal, and worth to be discussed. Is up to OP to decide to share the detail with someone impartial to clear this.
    – Lesto
    Commented Mar 8, 2019 at 19:35
59

"Inappropriate" might not be the best word, but "not strategic" would likely be accurate.

As what sounds like a perhaps still relatively new worker in a technical field, one of the first things you will need to learn is that decision making about how to do something, and when it is worth changing it, is not a simple matter. Given that you have an impetus to change something that already worked without being asked to, you are likely to find yourself often accused of "worshiping the new and shiny" without understanding the cost of change, complex reasons why something was done the way it was, or the full scope of new issues your idea would introduce.

Or maybe they're just small-minded people who found you annoying.

The thing is, to an extent, it doesn't matter what is objectively best, it mostly matters what is subjectively best for an organization at a given point in time. Change has a real cost in breaking existing awareness, so a new method needs to be substantially better in ways that matter and not just a little better in theory or a little more aligned to contemporary trends and thinking.

If you want to "volunteer" on something without being asked to, you'll likely get better reception for tackling real outstanding bugs that impact users, than in making bold re-writes of things which already worked. If you come to understand an unresolved issue, see if you can make a change that is as small and minimal a diff as possible, with a first class commit message. Make it obvious why this one change is the right way to solve the problem, and make it one that fits seamlessly into the current style and methodology of the code. Give them a pull request that is easy to approve and does not invoke any complex feelings of tradeoffs.

If you truly believe a section needs to be re-written, save that thought until you are being asked to contribute and are aware of priorities, history, and the nature of the codebase overall. And be prepared to understand why the change you want to make is not consistent with their priorities and plan. Somewhat counter-intuitively, the more you can demonstrate understanding of the current code by making fixes that fit seamlessly with its traditions, the more likely you are to gain trust to take things in new directions. You can also casually float drastic changes in a more informal way - "hey I was thinking we could make this part a lot better if we re-wrote it to use spindle folding" and gauge the reaction before actually doing it.

2
  • to an extant -> to an extent? Commented Mar 6, 2019 at 15:44
  • 8
    This is a very good answer, especially the fourth paragraph. There are few things as frustrating as people who want to change everything without understanding the rationale of the current processes/ systems/ rules. (And I'm writing that as a person who loves changes and sees critical thinking skills as one of the biggest strengths anybody can have).
    – BigMadAndy
    Commented Mar 6, 2019 at 21:05
34

Speaking from the other side of the desk - on a personal level, I'm quite happy when an applicant even has Github repos or some other kind of portfolio, and has done some background research on what the company does. This is like 3-5% of all applicants.

An applicant who potentially demonstrates both of those very directly, by fixing / improving our code? They probably missed a great hire, and you certainly avoided joining a terrible culture.

6
  • 13
    I'm curious, what is your role? I can see this from two perspectives--The candidate is a go-getter. From HR, that's golden right? On the other hand, it's exactly as the (technical) recruiter said: OP gave the impression of being THAT pain in the butt new grad who doesn't understand boundaries and wants to try to change things to how their professor said it was supposed to be (or worse, to how it makes sense to them). Ah, the paradox of wanting to hire proactive people who don't stick their nose where it doesn't belong...
    – Mars
    Commented Mar 7, 2019 at 4:41
  • 10
    @Mars IMO "pain in the butt" who finds problems early but annoys people in the process is better than "professional" who just sticks to their own borders. But my team is <20 people. I'd imagine this approach would have scalability issues in larger teams. My team often corrects me on things, since each of them knows their own area better than I know it, and is more up to date with tools and processes in their area. Commented Mar 7, 2019 at 10:14
  • 2
    @MarkKCowan For sure, a pain in the butt who finds problems early is great and worth their weight in gold. But judging from the reaction from the tech interviewer, OP didn't find a problem, they wasted their time (and the reviewer's time) with a PR that didn't contribute anything meaningful. I wouldn't want to pay an employee who uses company time in that way! (Sounds harsh and I'm sure none of us have been 100% productive in our careers, but when that's your first impression, they probably aren't going to think very highly of you)
    – Mars
    Commented Mar 8, 2019 at 0:25
  • 2
    @Mars But the OP (fresh grad) was not in the employ of the company. So, any time wasted on (what is potentially) a frivolous PR, is done by an employee already in the employ of the company. As such, any time wasted was done by an employee (possibly 2 if the they involved some HR guy about the candidate and had a meeting/discussion about it). This would tell me that the companies employees are in the habit of making sure they'll stay employed by employing unmaintainable code. Candidate avoided a walking into a volcano.
    – rkeet
    Commented Mar 11, 2019 at 13:51
  • 1
    @rkeet How do you know when a PR is frivolous without looking at it? The reviewing employee didn't waste time, they did what is likely within the scope of their job. It is not their fault that the PR they had to review was frivolous. There was also nothing in the original post to suggest that the employees were employing unmaintainable code, hence the selected answer that the PR may have just been frivolous. Be careful not to project too much!
    – Mars
    Commented Mar 12, 2019 at 0:48
22

You did nothing wrong. If a Pull Request that refactors one function of code rocked their boat, that doesn't leave a lot of room for more complex interactions.

The role of the project maintainer (or Reviewer) is to disentangle any politics (perceived arrogance, incompetence) from the code itself, and review the code objectively. If a reviewer receives a Pull Request and only focuses on the politics ("How dare you raise this PR?") and doesn't even review the code, they are being very ineffective in their role.

To be honest, it doesn't sound like they are looking for someone of your calibre, be happy you'll join a better company soon.

14

As @Kyralessa said, it may have been your comment not your PR
What did you enter as the comment to your pull request? That is the key missing piece here. You may have unintentionally communicated in your comment that the original developers were idiots and that you were far superior. The key word here is "unintentionally". As a developer its very easy to do. I'm not saying you did this, but it's a definite possibility.

...Or they're scared of dealing with a fresh out with initiative
Another possibility as others have mentioned is that they're overprotective of their code, and perhaps they don't want to deal with mentoring a fresh college grad with initiative who is going to need (as with all others in the same boat) significant mentoring and oversight to make sure they don't make any huge blunders (I speak from experience here having been one of them years ago).

...Or they don't know how to interview
They may just not know how to interview for the type of candidate they want and botched their side of the interview process.

5
  • 2
    Hey, I humbly advise against using h1 headers in your answer like this; it pollutes your answer and the entire page.
    – Marc.2377
    Commented Mar 7, 2019 at 0:56
  • 5
    Thanks for the feedback. Personally they help me with skimming, and they seem to help others as I've noticed they seem to increase the likelihood of getting votes (probably because people are more likely to read it). But I can see why some people might find them visually bothersome.
    – bob
    Commented Mar 7, 2019 at 14:20
  • 2
    Sure thing. While we are at it, note that a web page is supposed to have only one level 1 header (in the case of this page that's the question title, as verifiable in the source code). That said, I see no harm in using either h2 headers or bold text if done judiciously.
    – Marc.2377
    Commented Mar 7, 2019 at 14:36
  • 1
    Sorry realized that my answer has been edited as described in your comment--I understand a little better now. Sure I can stick with bold text--I do think it looks good.
    – bob
    Commented Mar 7, 2019 at 14:56
  • 2
    My own company just rejected a superficially skilled candidate on that basis. We know he's smart, but the tone he took to report the need for better performance of a game he was using really turned us off. We don't want him talking like that to anyone at the company, and life's too short for us to take the risk of having to raise him properly.
    – user90842
    Commented Mar 11, 2019 at 21:41
9

In most companies, your actions would be seen positively even if there was a good technical reason to reject your pull request in the end:

  • It shows your genuine interest in that position in particular
  • It's an evidence that you have hands-on experience with coding
  • It was an opportunity to talk about real code during the interview, instead of made-up coding exercises

That is, unless the code you wrote was complete nonsense that convinced them that you didn't have the experience they assumed you had from the first interviews, or you somehow managed to insult them in the comment.

2
  • The other possibility is that the repository had a obvious and maintained issues / feature log and OP ignored all of that to rewrite a bit he thought he could show off with that had no issues and no feature requests. Which could actually show a lack of interest in being useful.
    – J.Doe
    Commented Mar 7, 2019 at 16:24
  • @J.Doe For me that would rather mean a lack of experience with GitHub, which (unless the CV advertises the OP as a GitHub expert) is a minor annoyance, not a reason to drop the applicant. Commented Mar 8, 2019 at 9:04
6

He said that it came across like I know more than them as a fresh college grad, and that I haven't considered why they coded it how it was. I didn't end up getting the job.

Consider yourself lucky for not getting the job, because the treatment you got for this pull request is probably a taste of the treatment you would have gotten if you worked at that company and proposed this (or any other) change.

To be perfectly clear: Yes, I find it very likely that your PR wasn't a good fit for them and that they genuinely have good reason to have their code the way they do, instead of the way you proposed. In other words, I very much believe that your code was probably simpler, but worse.

However, unless you included a rude comment in the PR, the senior's assumption that a simple suggestion is "inappropriate", that it is an arrogant way of saying "I know more than you", and that a college-educated candidate cannot, in fact, know as much as them or more, is a triple red flag because:

  • It raises suspicions that if you worked there, even your good ideas would be dismissed entirely on the grounds that you are a junior, just so the seniors can justify their own role and paycheck.
  • It demonstrates that they have some serious insecurities about their own expertise — and I would be inclined to think that those insecurities might be justified.
  • If that senior happens to lack formal education in software, there is added incentive for them to try to downplay the importance of a degree and the expertise one gets out of it, lest their own managers eventually replace them with equally experienced developers who also have certifications.

Just to give you some perspective, I once interviewed somewhere and in the process I made a somewhat radical suggestion to the seniors about a system they were in the process of building. Not only did they welcome and consider it, but they also made me an offer shortly thereafter.

Such environments do exist — not all companies employ a one-way teacher/student model where knowledge flows strictly from the seniors to the juniors. (And remember, if you have graduated then you are not a "student", and many seniors in this industry aren't actually "engineers" either, regardless of what a company decides to call them.)

2
  • I would equally stay away from places that implements everything you suggest. I seen cases where everyone's idea is implemented and it ends up with a horrible code base that is impossible to modify. I would want to go to a place where you have proving grounds and once you reach that point, your suggestions and opinions are valuable to everyone equally. At the very least, I would respect the hiring manager had he said the idea was good but they cannot do it for business reasons and then go in depth with why the OP chose to do it and see his thinking process. Not outright end the interview.
    – Dan
    Commented Mar 11, 2019 at 17:34
  • @Dan I'm not talking about whether they accept or reject any one of your suggestions. I'm specifically talking about whether a junior isn't even in principle allowed to suggest anything. To me, this screams that the seniors employed there are probably impostors — and even if they aren't, it's best for a junior to go to another company where they won't have their enthusiasm demolished.
    – Natural20
    Commented Mar 11, 2019 at 18:13
5

The problem is, your "improvement" was naïve and artificial, and I know that because of how much shorter you were able to make it.

This happens all the time to me. I build a complex system to allow data to serve many users. And then someone comes along and says "We don't need all this schtuff! You're making a simple problem much too complex." And they hack and slash, and reduces it to a simple system that serves them well, and they give themselves a gold star.

Except they are not the only user. And the modifications just broke it for all the other users of that data. So then there has to be a thing... meetings, re-education, bitterness, rollbacks, all of which was unnecessary.

Coding is the easy part. The hard part is understanding and expressing the problem. You short-circuited the hard part, to make the easy part easier.

If you were taught that coding is king, well, not really. The other side is where the money is: being able to write a spec that is codeable and handles all users/needs. (on the other end of the scale it's also possible to design solutions which are all-singing/all-dancing, but unwritable, that's why you need to know coding to design).

That was the core of it. You didn't fully understand the problem the code was trying to solve.

And you displayed that to them in spectacular fashion.

In gaming, a "newbie" is a mere novice. A "noob" is a novice whose arrogance prevents them from learning, or respecting others' experience or their elders generally. It seems like the latter is more applicable to you, because you were so easily able to make that code so much shorter, and it didn't occur to you that this was too easy, there had to be a reason they had written it so complex.

4
  • 2
    +1, you nailed it. I think a lot of people on this board are under the impression that coding ability is the key to success in software development, and it isn't. It isn't even the most important skill. Understanding the problem and what problems actually need to be solved is so much more important. OP failed to demonstrate this with his PR.
    – Seth R
    Commented Mar 7, 2019 at 18:52
  • In gaming a noob is the same thing as a newbie. Noob just takes less time to write. Even if he didn't understand the problem, he's not even a new hire at this point. Just take the interest as a plus and reject the request.
    – user53651
    Commented Mar 7, 2019 at 23:15
  • 5
    @Harper -- you're making a lot of assumptions in your answer. Also, if your code is that complex, it should probably be decoupled/refactored/rethought a bit more than it is. Complex does not necessarily mean complicated. And I've worked through quite a bit of code, written by people who stuffed it all in one place, that was much easier to scale, maintain, and understand, after it was gutted, simplified, and put back together. Commented Mar 8, 2019 at 4:38
  • @Harper - so much speculation here. I found the PR and included it (obfuscated) in an answer and it's pretty clear that it does not at all match the many assumptions you made. Commented Mar 14, 2019 at 19:38
3

It's hard to se how it could be intrinsically inappropriate to write a PR for anyone's open-source project.

Therefore it must come down to the particulars, of which we know very little. Was it naive or arrogant in code or attitude? Was it helpful and friendly? Without knowing more it's hard to gauge the appropriateness.

Curiosity got the better of me. I found your PR. And it made such an impression on me that I decided to share it here. It was not a light decision because I don't want to betray the confidentiality of neither you nor the company, but I felt it would bring substantial context to the discussion in an acceptable way. The lack of concrete details has surely lead to much unsubstantiated speculation

I have completely anonymized and obfuscated the PR by changing all custom variables, strings, methods, and comments. Here it is, in its entirety:

  # if this is invoked with an argument then use that for target
- target = 'jadaskjafjldfsfsasf'
  if len(sys.argv) > 1:
      arg = sys.argv[1]
      if arg == '...':
          print '...'
      else:
          target = arg
-
- match = some_lookup(target)
+         match = some_lookup(target)

  if match:
      print "..." 

The code will initialize target to a hardcoded random string. (Note, I only shuffled the string's characters to obfuscate that part). If an argument is not provided then some_lookup(target) will fail to produce a match because it presumably cannot lookup the intentionally wacko default string.

This is clearly by design. But it is also objectively bad coding.

Your fix seems like an improvement. I myself would remark on this in a code review, without hesitation. And I could easily see myself writing the exact same 25-word-long friendly, non-confrontational commit message that you wrote.

Therefore this particular PR does not seem inappropriate to me. And provided it's done in a professional, respectful manner and in good faith, a PR would never be inappropriate including when interviewing.

1
  • 1
    Great answer!! I'm not familiar with that language, but the change does look like an improvement to me as well (and keep in mind, that my default is usually not to change anything, or to change as little as I can). It looks like the OP dodged a bullet with this PR. The OP should be patting himself on the back for what happened after that. This should be considered a victory, not a failure. The real failure would have been finding this out after you've started the job with them. Commented Mar 25, 2022 at 2:41
2

and that I haven't considered why they coded it how it was.

Yes true. In some cases, code is written to support a particular business function or rule that the programmers cannot control.

I looked at it for a while and found a much simpler and future-proof way to write one function, and I opened a PR with the change without mentioning that I was currently interviewing.

As a young person, we like to think we're clever. That we figured it all out. The truth is if you thought of it, someone else might have as well since you obviously "found" a better way by googling what other people did. Whenever you think of something so blatantly obvious, you should stop and ask first about it to just make sure what is being accomplished in the current way. Bill Gates didn't google his way to build Windows, he thought of it and implemented it. Unless you are able to do the same, you haven't found a "better way." You just google'd better than the last person.

Was it really inappropriate for me to do this?

As a PR to their master, yes it was slightly inappropriate. Perhaps a branch of your own that you can share at the interview. "I saw how you did X and upon researching I found Y that allows for future proof and easier modification. I know you guys wrote it for a reason but I was curious to demonstrate a concept based on your code." I know in git you can use @ symbols and even open up a discussion chain. Perhaps it might be best next time to comment the section you modified with a,

@user I notice you guys are doing X here, but I put in a Y. Wanted to show my ability to read your code and make modifications,etc

By making a PR to their master, it's essentially the same as the news story of the man who wanted to be a plumber, couldn't find a job, so decided to "fix" a gas leak in his neighborhood. You can imagine the end result that happened.

3
  • 3
    -1 for the last paragraph. An amateur 'fixing' a gas pipe could cause untold havoc, but a pull request doesn't make any changes to the code it's based on until it's accepted. It's more akin to calling the gas company and saying "you have a leak because a tree's roots have crushed the pipe; you can put your pipe through my garden to avoid the tree if you like" Commented Mar 8, 2019 at 9:23
  • 1
    @RichardWard The PR did have real life implications.... he failed his interview because he offended the developer. It wasn't his intention as he wanted to show off his skill, but it did have a side effect he did not intend. His PR was probably rejected on top of all that.
    – Dan
    Commented Mar 8, 2019 at 14:09
  • 4
    Is that not more like the gas company saying "we don't want your garden, and in fact because you offered it we're going to cut off gas to your house"? Commented Mar 8, 2019 at 14:30
1

To add to other answers,

are you sure your code was indeed correct and useful in that particular codebase?

You fix may seem much simpler and more robust; however it may easily be that the old code was written the way it was written on purpose.

Probably your pull request changed some aspects of the behavior (you might even think that you fixed a bug), but there is some distant code that relied on that bug.

Probably you did not account for that way the code was used, and therefore you code is worse in that particular situation. For example, you code might be not working in multithreaded environment, or the data that the code deals with may have some unobvious properties that make the old code work better and faster.

For all we know you may have overlooked some silly reason (a bug in your code, or the fact the your code runs slower, etc.) that should be obvious for an experienced developer.

You may have overlooked something else. After all, they are working with this code for a long time, and should probably know much better how it works. The fact that they said "that [you] haven't considered why they coded it how it was" suggests this.


This said, I agree with other people who say that opening a PR is nothing bad. However, as with any new codebase, often it is better to discuss it with the maintainers. Given that you were in the process of interviews at that time, you could simply have raised that question on the interview.

2
  • 1
    if a bug was left unfixed intentionally, wouldn't it be appropriate with a comment saying something like "we know 1+1 shoulnd't equal 3, but we break x and y if it doesn't"? Just to avoid this exact situation where someone unfamiliar with the codebase 'fixes' it?
    – Atheist
    Commented Mar 7, 2019 at 11:15
  • 2
    @Atheist, in an ideal world — yes. In reality — by far not always
    – Petr
    Commented Mar 7, 2019 at 11:38

You must log in to answer this question.