Skip to content

Conversation

English3000
Copy link

@English3000 English3000 commented Dec 9, 2018

Frankly, I'm a little confused by your reasons against refactoring, namely because I just haven't seen them before.

In terms of "messing up history", is there some special view on GitHub for that? Otherwise, I don't get it...

Whatever the case, I don't expect you to accept this one either (it's incomplete)--rather just to produce something in this vein.

retriever.ex in particular is nastily complicated and without a doc like this, I'd be very surprised by open source contributions (albeit, I think the groups_for_functions PR was, so...).

@josevalim
Copy link
Member

josevalim commented Dec 9, 2018

In terms of "messing up history", is there some special view on GitHub for that? Otherwise, I don't get it...

For instance, if we want to git blame a piece of code to understand why a decision was taken, now we need to go through all the changes that simply moved code around without changing its behaviour. It is for instance one of the arguments made against the formatter (or an argument to format everything at once so the history is messed up only once).

To be clear, git blame is not a reason against change. But if we want to move code around, the gains have to justify its costs. Imagine what would happen if every 6 months someone decided to reorganize the codebase? We would have tons of churn and people who actively contribute to the repository would need to relearn it every time.

Not only that, reviewing all of those changes are very time consuming, which should also be taken into account. For example, take this PR from @eksperimental that improved autolink. We had to go through multiple arounds of reviews and back-and-forth, which is time consuming for everyone. But at least that PR was focused and had a clear goal, which made it worth it.

Even me typing this and you reading it takes both of our time and any of us should be able to prioritize our own time and sometimes that means focusing on other areas.

I'd be very surprised by open source contributions (albeit, I think the groups_for_functions PR was, so...).

While improving the contributing experience is always welcome, this project has had 97 contributors according to GitHub. The retriever.ex file in particular had 25 contributors (+25% of the total). So saying you would be "very surprised by open source contributions" is misleading and honestly, a bit off-putting as it is a hasty generalization.

In any case, we will be glad to merge contributing steps, assuming they are complete. So in case you or someone else want to finalize and format this, please let us know and we will review it.

@English3000
Copy link
Author

Re: git-blame, I just added it on Atom and it doesn't seem to appear in chronological order. Also, it seems there are duplicates of the exact same commit. Is there a way to fix those? (Don't see anything about chronological order on a quick search.)


Re: the scope of this PR, a few months ago, I was trying to add some more customizability to the nav bar (specifically, the ability to group functions by category, as is already done for modules), but as I said, the way the codebase was written and the lack of documentation made it hard to wrap my head around... and making this more readable in order to add that feature (since no one wanted to help me with that feature...) was the reason I made this set of PR's.

While I do see plenty of commits from different people, I'm not sure we mean the same thing by open source contribution. I'm speaking to a feature-level contribution. For example, in docs.ex, the normalize_*_url helpers are very similar so I turned them into 1 helper, normalize_urls, using Enum.reduce/3. That would technically be open source (if I had stayed with the concision approach), and could account for a number as high as 25%.


Re: being time consuming, I mean it's obvious (and reasonable) that's the true reason you are closing my PR's. I'm just confused by your implication here that I'm the "only one" (or in the true "minority of people") having this experience of your codebase. You yourself said retriever.ex was a pretty nasty file:

Also, keep in mind there is no guarantee a contribution will be accepted, especially if it will make the retriever much more complex (as you already noted it is complex today).

So wouldn't the benefits of a more readable retriever.ex speed up the pace of development for the whole team?

To me, a lot of this just sounds like codespeak (no pun intended) regarding our different approaches/code styles. So while it's in your power to reject my PR's outright, I'm a little confused why the response wasn't:

@English3000 In your PR's, you change code style along with refactoring. This makes your PR extremely time-consuming (and difficult) for our time-constrained team to review.

If you would like to contribute, I suggest you attempt to make the changes in a manner that changes the codebase as little as possible while achieving your desired result.

I mean, that's what you're insinuating (along with that refactors, per se, don't add value).

And my response is that doing that may make the codebase leaner, but it won't make it more readable (for someone who didn't help create the codebase).


Re: CONTRIBUTING.md, I'm sure you'll understand that I'm a little skeptical about going further with it. You yourself already stated that the team knows the codebase as is better than I do, so wouldn't it make more sense for one of them to do it?

And if I did do it, you could simply change it after all my disproportionately hard work to a format that I didn't even find helpful.

If you like the idea, go ahead, but the tone of your communications has been you don't want my contributions here, and actions speak louder than words.

@English3000 English3000 closed this Dec 9, 2018
@josevalim
Copy link
Member

josevalim commented Dec 9, 2018 via email

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