Skip to content

Conversation

English3000
Copy link

Motivation

A few months ago, I wanted to add a new feature to :ex_doc, so I made a clone. However, the call stacks in the files I needed to change ran so deep that for someone unfamiliar with the codebase, they were unreadable. Unfortunately, the team did not have the bandwidth to guide me along.

As a result, with a bit more free time now, I figure the natural next step is to solve that problem: readability.

Description

This is the first of many PR's to come. If the team disagrees with my initial effort, no worries! I have created my own fork, so if you'd prefer, I can just offer a PR with the final product.

In this PR, I refactored the root file, docs.ex, for the feature I want to add.

I took 2 approaches. In the first commit, I simply refactored for concision. Then I thought to a conversation from work about code style. The result was the second commit.

The comparison is best explained with the metaphor of a book: When you open a book, you could go to the Table of Contents to see its overall structure and what it is about. Or, you could skip to the first Chapter and just start reading.

Your code (and my first commit) followed the Table of Contents approach. While this makes for slightly easier-to-refactor code, it makes it very hard to follow what the code is actually doing because someone new to the code has to jump all over the file, losing track in one's mind of the bigger picture. So, the Table of Contents approach is creator-friendly.

By contrast, in this PR (which follows the Chapter approach), you can almost read the file as you would a book, starting from the top and finishing at the bottom, with notes to the reader in the few places one does need to jump.

The pros of this approach are a leaner run/1, which is less intimidating and easier to understand at a high-level (without the distraction of the Table of Contents), and an easy-to-follow call stack using tail-calls to maximize readability.

Given the main pro of the Table of Contents approach (slightly easier to refactor), I did leave a comment with it above get_docs_opts/2 in case a maintainer needs to reason with it in making a significant change. However, given that :ex_doc is a relatively mature project and that this file has a specific purpose which is unlikely to change significantly, at this point I think it makes more sense to make the code more accessible for open source contributions.

Let me know what you think!

@josevalim
Copy link
Member

Hi @English3000,

Thanks for the PR but we tend to not accept general refactoring of the codebase to avoid code churn, messing up the history and so on. So if there is a particular function that you think could really benefit from a refactoring, then a PR for that function would be appreciated, but a general refactor wouldn't be merged.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants