Contributing code for the first time
Published: 2023-09-21 13:19:46

For week 3 of my course [Topics in Open Source Development](https://github.com/Seneca-CDOT/topics-in-open-source-2023) at Seneca College, I was tasked with working a partner to review each other's code. We looked at each other's repositories: 

My repository: https://github.com/paulkim26/til-to-html

My partner's repository: https://github.com/mismathh/TILerator

## Identifying Tasks

The first step was to submit an [issue](https://github.com/mismathh/TILerator/issues/8); I intended to enhance mismathh's tool to:
- Handle Markdown (.md) files
- Parse Markdown bold text as html `<b>` tags

I looked over the code and realized there were multiple ways to approach this. I could simply append the new functionality to the existing files or I could make a new module that encapsulated the new functionality. Then that lead to more questions like "should I create a new folder to contain my new code" or "what should I name these new files/folders". I asked and mismathh thankfully clarified I should implement the functionality within a new module. Had I not asked, I could've made a bad assumption I'd have to revert down the line.

## Contributing Code

I forked the [repository](https://github.com/paulkim26/TILerator) and started working on a new [branch](https://github.com/paulkim26/TILerator/tree/issue-8). I was careful to refrain from changing the code too drastically, changing things that were outside of the scope of my submitted issue, and adhering to mismathh's coding style - even if I personally did things differently in my own projects.

For example, I noticed there were a lot of index based `for` loops where I would have personally opted to use `for...of` loops or use methods like forEach, filter, map, etc.

Regardless, the code was very readable; I commend mismathh for coding in a manner that was straightforward and self-documenting with additional comments inserted where needed. I added a new file called [`markdownToHtml.js`](https://github.com/paulkim26/TILerator/blob/issue-8/src/markdownToHTML.js) that encapsulated my new markdown conversion functionality and imported it into the existing `utils.js` file - that helped keep things readable.

When I was ready, I pushed my branch to GitHub and created a [new pull request](https://github.com/mismathh/TILerator/pull/9). It felt a bit repetitive to describe what I had changed all over again and reiterate my commit messages in the pull request's description, but I suppose there's no harm in being **too** verbose and explicit with my intentions, especially since this was not my codebase to start with. 

## Pull Request Feedback

mismathh looked over my pull request and responded with a request for a change - it turned out that I had introduced a new bug.

I was using the `split` method to extract the file type from a file path name, for example:

```ts
const filePath = "examples/til.txt";
const fileType = filePath.split(".")[1];
```

In this case, `fileType` would correctly return `"txt"` as expected by splitting the filePath string by the `"."` character and returning the string that was after it.

However, this was predicated on `filePath` being written with **1** period - this was not a robust solution.

mismathh noted this would not work for all situations:

```ts
const filePath = "./examples/til.txt";
const fileType = filePath.split(".")[1];
```

In this example, `fileType` would resolve to `"/examples/til"` rather than `"txt"` as expected. He suggested using the `path.extname` method which was explicitly designed to extract the extension name from a file path. This proved to be a much more robust solution that worked great! I implemented his solution accordingly.

## Pull Request - Lessons Learned

From this experience, I learned to:
- Use existing methods/functions/APIs that are custom suited for the problems I'm trying to solve if possible.
- Consider how others may test and run the same code differently from me.

Next time, I should think more carefully about a bug report that may appear to be a false positive to me and see how I may be biased in my perception of it. mismathh pointed out that I should use an index of value 2 (`const fileType = filePath.split(".")[2];`) - this should've tipped me off that there were test cases I had not accounted for instead of assuming mismathh had a mistake. He had gone out of his way to report this; there was a user story I had yet to discover.

## Receiving a pull request

On the opposite end of this experience, mismathh submitted a [pull request](https://github.com/paulkim26/til-to-html/pull/13) to my repository. I really appreciated how explicitly mismathh detailed his planned changes to my code, that alleviated a lot of my initial reservations about having someone touch up my work.

For this pull request, he submitted 8 commits, I really appreciate that not only did he split his commits into digestible, sensible scopes, but he inferred my commit naming convention ([Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/)) despite me not stipulating so and adhered to it. In hindsight I should've made a note in my README about following this naming convention - I got lucky that mismathh is just that awesome.

## Reviewing the Pull Request

I pulled the new branch into my local machine and started by just running the code through all of my test cases. Everything worked great. Then I checked all of the new code - mismathh was careful to adhere to my style and project structure, even separating his new markdown header parsing function into its [own module](https://github.com/paulkim26/til-to-html/blob/main/src/parse-markdown/parseHeadingTwo.ts), as I had similarly done.

All in all, the changes were solid, they worked great and were readable. I made a couple of suggestions that adhered to my stylistic preferences such as:
- Using `const` instead of `let` whenever possible
- Eliminating a redundant iteration in a `for` loop
- Renaming a variable
- Eliminating a redundant template string

So, very small changes. He quickly implemented all of my suggestions and I accepted and merged his pull request.

In my head I was thinking "I could just accept this pull request without revisions and call it a day" but I'm glad I didn't and asked for the changes I would've made on my own. mismathh was receptive to feedback and I could simply move on to other endeavours after the pull request was submitted.

Thanks for reading!