Skip to content
This repository was archived by the owner on Nov 19, 2022. It is now read-only.

implement-an-exercise-from-specification: Remove avoiding duplicate work section. #108

Merged
merged 1 commit into from
Nov 5, 2017

Conversation

Insti
Copy link
Contributor

@Insti Insti commented Oct 31, 2017

The Avoiding Duplicate Work section encourages people to create empty WIP pull requests.

But:

  • Most of these are never followed through on and become more of a
    problem for track maintainers than than the far less likely
    implementation collisions.

  • Tracks should be able to specify their own procedures for creating
    pull requests.

So:

Remove this section from the common documentation, if tracks think that
it is important to have WIP commits they can add them to their local
track documentation.

This section encourages people to create empty WIP pull requests.
But:

* Most of these are never followed through on and become more of a
  problem for track maintainers than than the far less likely
  implementation collisions.

* Tracks should be able to specify their own procedures for creating
  pull requests.

So:

Remove this section from the common documentation, if tracks think that
it is important to have WIP commits they can add them to their local
track documentation.
Copy link
Member

@ErikSchierboom ErikSchierboom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really like WIP PR's. They can be useful, but they should really be the exception.

@petertseng
Copy link
Member

I understand that we shouldn't draw conclusions from two isolated incidents, but I will share them in case other incidents indicate a trend.

In both, the empty commit resulting from the WIP PR has caused confusion for a reviewer of the PR. It could be the case that the empty commits are unintuitive, or that the reviewer is not familiar with the practice.

The selection of anecdotes provided by this comment is biased, because no attempt was made to find instances where the WIP commit avoided a conflict.

@Insti
Copy link
Contributor Author

Insti commented Oct 31, 2017

no attempt was made to find instances where the WIP commit avoided a conflict.

This is a difficult, if not impossible task.

If, once this PR is merged, we are met with a flood of conflicting PRs we can address that issue.

But I very strongly believe that will not happen.

@NobbZ
Copy link
Member

NobbZ commented Oct 31, 2017

I am strongly against a simple empty WIP-commit with an empty PR.

I do not have anything against WIP-PRs in general. But over the last month I got 2 PRs on erlang erlang which started out with empty commit, both without any (meaningfull) PR description, I was happy they mentioned at least the exercises name in the subject…

I prefer if the first commit of a WIP-PR does at least contain the files that make off the exercise (readme, example, tests, stub, etc; the files that are crafted in the process of implementing may be empty for now). Also I really appreciate if in the PR description a rough estimate is given about when we have to expect the first reviewable item, such that I can integrate it better into my daily routine and plan ahead.

I already stated this in the erlang readme, that I want to have this estimate when “obviously unfinished code” is submitted. I'm thinking about hardenforcing this rule with a quote and close, but I do not want to scare the few contributors away that are left…

@Insti
Copy link
Contributor Author

Insti commented Oct 31, 2017

This appears to be the only place in the Exercism documentation where creating a WIP commit is mentioned. Source Github search

So I guess it's good that people are reading the documentation, but they'll presumably still do that even without this text.

@rpottsoh
Copy link
Member

rpottsoh commented Nov 1, 2017

Is it just OK to remove that section of the doc just because we don't like WIPs? Would it be preferable to suggest opening an issue in the track's repo to announce that work has started on porting an exercise or to perhaps refer the contributor to the documentation provided in the particular track they are looking to contribute to? Just thinking out load again.

@Insti
Copy link
Contributor Author

Insti commented Nov 1, 2017

Would it be preferable to suggest opening an issue in the track's repo to announce that work has started on porting an exercise

No. Tracks should be able to give their own guidance on issues and pull requests.
Having such instructions in the common documentation leads to tracks having their process dictated to them rather than controlling their own.

refer the contributor to the documentation provided in the particular track they are looking to contribute to?

I believe this is already present in the document.

Copy link

@ilya-khadykin ilya-khadykin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've also had some negative experience with this approach.
I guess there is a better workflow for this

@rpottsoh
Copy link
Member

rpottsoh commented Nov 4, 2017

Safe to merge or is the PR in some ways dependent on feedback @Insti is looking for in exercism/ruby#762 (comment)?

@kytrinyx
Copy link
Member

kytrinyx commented Nov 5, 2017

I'm fine with merging this. I introduced this idea because I was seeing a enough people do the same work at the same time, that it seemed like a good approach. I use WIP PRs at work, and have done in several jobs—very successfully. These are typically about getting early feedback, though, since we don't have random contributors picking up work without mentioning it to anyone at work.

@Insti
Copy link
Contributor Author

Insti commented Nov 5, 2017

Safe to merge or is the PR in some ways dependent on feedback @Insti is looking for in exercism/ruby#762 (comment)?

This PR can be merged. It is unrelated to that one.

@rpottsoh rpottsoh merged commit 798d812 into master Nov 5, 2017
@Insti Insti deleted the Avoiding_avoiding_duplicate_work branch November 5, 2017 16:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants