In previous blog post I’ve continued talking about the challenges associated with contributions to a huge Open Source project like Thimble. During this release, I continued working on the same piece of code.
After an extensive review and a huge amount of feedback, my initial code was completely discarded and a totally different road was taken. My first implementation was focusing on adding functionality to the existing code by creating a new module that will be called together with every filesystem event. There were a number of problems with that approach:
- The code required a whole bunch of snippets to be added in different parts of the existing code. This would have made if a real ordeal of maintaining the new code.
- The resulting code was now really extensible.
- The solution was kind of brute force, which conflicted with the coding philosophy of the project.
So with the careful guidance of David Humphrey, we were able to narrow it down to the desired procedure. Instead of making my module a colleague of the bramble instance, we make it a decorator of the filesystem, represented by the Filer object. Here is a simple diagram that show an example with the “writeFile” function:
- fs_ – a filesystem instance that bramble communicates with that is actually a ProjectStats object.
- ProjectStats – a decorator that wraps around the Filer object. It does additional bookkeeping of current project size, and forwards the request to the Filer.
- Filer – the actual object that controls the filesystem.
With this design in mind, we are able to solve all of the previous problems. Code insertion is now kept to minimum, just enough to accommodate the decorator. Here is an example.
The ProjectStats now contains only to state variables:
- Reference to the Filer
- Cache that consists of key-value pair of all pathnames of all project’s files and their size.
On every call of the function that is supposed to modify the project tree, cached value for the corresponding file is created, updated or deleted. Later, when the we need to know the total project size, cached is reviewed and all of the file sizes are added.
Although, the explained solution is easily written, the styling aspect of the code posed the biggest challenge by far. If you take a look at my pull request, you will find almost 140 comments. The page even takes a considerable amount of time to load because of them.=) Most of the comments are requests to fix styling of my code. I can’t even remember how many times I needed to “fix indentation”, “add space in between the function parameters” or “delete this empty line”. As annoying it was for me to update all of that, I can only imagine how tired my reviewers were. Mainly David Humphrey. Quote by David:
@gideonthomas if you want to review this, please do so soon. I want to finish this.
I mean, 130 commits down the road, he was pretty pissed off and tired… So to ease his pain, I introduced a bunch of styling rules using Eslint based on feedback I received. I would have shown you a snippet, if I didn’t squash all of my old commits into a single result. You will need to believe me when I say that there were around 25 rules.
After all of this, my PR finally landed, but that was not the end of it. I needed this code for my other fix on the Thimble side. So I tried using it, and it worked fine until I logged in to the app. Once logged, the whole file tree crashed and stopped responding. I will admit it, at the time I panicked. Imagine me, a newbie developer that sneaked his code malfunctioning code into a major system. I was terrified! So I asked David to revert my PR until I can investigate the cause. An evening of futile attempts only made me feel worse. On the next morning, I was able to pin-point the problem. MY VW WAS OUT OF DATE WITH THE CODE IN MASTER!!! When I came across this problem, I was to concerned, that checking the integrity of the master branch sloped out of my mind. In the end, my code was fine (Weeh!).
Now to talk about a less stressful part of this release. My second problem focused on notifying users about absence of index.html file in their project root. More details. Let me tell you this, returning to a simple html problem like that was like a breath of a fresh air. Two fast days of development ended with the following outcome:
This message would appear every time there is no index.html file in the root of the project, preventing consequent publishing from occurring. I am using code from Part 1 of this release to check for index file and display the message’s div accordingly.
So far my reviewers are content with the changes I have made. Hopefully, I will be able to land this PR soon.
Part 1 PR: pull request
Part 2 PR: pull request