Unsuccessful pull requests: why and how to avoid

It's been almost a month since hacktoberfest ended, but I've still been hacking away at my open source contributions. That said, not all my attempts at contributing were successful, and I'm going to highlight some some unsuccessful cases as well as some things I've learned from my experiences.

About the Repo

markdown-it is a Markdown parser that I have used before to convert markdown content to html. I chose this repo to work on since I was familiar with the tool and I knew some of its functionalities.

---

First Issue: Attempt

The first issue I worked on was an issue I raised when I discovered I could not successfully run the benchmark command outlined in the contributing docs.

I thought this was a small issue, and decided to attempt a fix and create a PR. I quickly found that there were some file paths in the code that reference non-existing directories.

In three places in the project code there are references to ../../extra/lib/node_modules, however this directory does not exist. The only place I could find a node_modules folder was ../../../node_modules.

When I corrected the paths, the benchmark tool run command worked for me! Therefore, I decided to raise a PR.

First Issue: Result

The project owner commented in my PR that the reason I could not run the benchmark tool was because I needed to first run a command npm run benchmark-deps to install the benchmark tool files into the extra directory prior to trying to run the tool. My issue and PR were promptly closed.

However, even after I ran the command and install the files, the benchmark tool still does not run. I had a brief discussion with the project owner in the comment thread of the closed PR regarding other things I could try to resolve it. Some things I tried include installing all dependencies with npm install and running on a different terminal such as git bash and Ubuntu. In the end, we could not resolve it, so he suggested that I created a new issue to track this problem.

---

Second Issue: Attempt

The second issue I worked on for this project was a pre-existing issue from August. markdown-it has an option to toggle on html mode, which means that it can support the parsing of Markdown content that already contains some html without error. However, the issuer has discovered that when the <pre> tag is used in multi-line html code containing a blank line, the output is invalid html.

Input:

<h1>Heading</h1>
<pre>
First line of text

Second line of text
</pre>

Output:

<h1>Heading</h1>
<pre>
First line of text
<p>Second line of text
</pre></p>

I decided to tackle this problem, since I had a clear way to reproduce the error. Also, it was very straightforward what the current output was and what is expect output should be.

It took me many hours to investigate this problem. For starters, I only knew how to run this tool from my npm install, and I didn't know how to run the code locally. I also had to find out which function or code block was triggered when html mode is on.

Eventually I did find out which code was triggered when we pass in this particular input.

// lib/index.js
...
var ParserBlock  = require('./parser_block');
...

// lib/parser_block.js
...
var _rules = [
  ...
  [ 'html_block', require('./rules_block/html_block'), ... ]
  ...
];
...

// lib/rules_block/html_block.js
...
var block_names = require('../common/html_blocks');
...
var HTML_SEQUENCES = [
  [ /^<(script|pre|style|textarea)(?=(\s|>|$))/i, /<\/(script|pre|style|textarea)>/i, true ],
  [ /^<!--/,        /-->/,   true ],
  [ /^<\?/,         /\?>/,   true ],
  [ /^<![A-Z]/,     />/,     true ],
  [ /^<!\[CDATA\[/, /\]\]>/, true ],
  [ new RegExp('^</?(' + block_names.join('|') + ')(?=(\\s|/?>|$))', 'i'), /^$/, true ],
  [ new RegExp(HTML_OPEN_CLOSE_TAG_RE.source + '\\s*$'),  /^$/, false ]
];
...

// lib/common/html_blocks.js
...
module.exports = [
  'address',
  'article',
  'aside',
  ...
  // many tag names (excluding script, pre, style, textarea)
];

I found out that the code in lib/rules_block/html_block.js was triggered when we try to convert our input, and I verified this by printing out some messages to the console.

From my investigation I could see that four tags (script, pre, style, and textarea) were treated differently from the rest of the html tags. If there are any blank lines inside these four tags, it will trigger the closing tag, i.e. </pre> tag, to come before the </p> tag.

I found if I just inserted 'pre' into the list of tags in lib/common/html_blocks.js, then this behaviour would not occur. However, I wanted to investigate why the code was written in such a way for this behaviour in the first place, since it seemed intentional.

Second Issue: Result

In the comment at the top of lib/common/html_block.js, I found an outdated link to the commonmark specs. (markdown-it follows the commonmark specs) I found the updated version of the commonmark specs and later submitted a PR to update this link.

According to the commonmark specs, this behaviour with the <pre> tag was expected behaviour!

Even though I had my code fix ready to go in my branch, I decided to not create the PR since there was little chance it would get accepted. I reported my findings in the issue. Issuer was glad to learn about this and project owner promptly closed the issue.

---

Conclusions

In conclusion, I found out there are many reasons a PR might be unsuccessful, including:

• Outdated documentation
• Inactive repo admin
• Duplicate issue
• Duplicate PR
• Non-issue
• Code quality
• Insufficient testing
• Poor communication
• Outdated changes
• etc.

Here are some things I've learned from my experiences. Keep in mind I'm still new at open source so this is a very short list!

• Communicate frequently (ask to get assigned, ask for feedback on raised issues, ask for feedback on how they want something implemented, etc.)
• Choose a repo that is active (has pull request merged recently)
• Sort for issues that are created by the project owner (less chance of encountering a non-issue)
• Ask for more details on an issue before starting to work
• Look for documentation and review it before starting to work

These setbacks have not deterred me from contributing to open source! Getting a PR merged is only a small part of the larger process I went through to read and debug the code, and I've definitely learned a lot :)