Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal: bracket-push: rename to matching-brackets #1501

Merged
merged 1 commit into from
Apr 15, 2019
Merged

Proposal: bracket-push: rename to matching-brackets #1501

merged 1 commit into from
Apr 15, 2019

Conversation

petertseng
Copy link
Member

Summary: Here is my official proposal to rename bracket-push, which wants your comments. We have gained experience from the retree->satellite rename that renames are safe to do. Reviewers of this proposal should carefully consider whether the proposed change warrants the costs that will be incurred by the 31 tracks that have implemented this exercise.

We can consider discussion on what the new name should be, but if nobody has any other options, I defaulted to matching-brackets. Another possibility I thought of is bracket-matching.

I have another comment coming up with necessary follow-up steps that would need to be taken if this proposal is ratified.

Below is the original commit message, which contains rationale for the change.


I rehash my prior allegations of
#693 below:

I contend that having the word push in the name unnecessarily biases
the solution space toward solutions that use a stack data structure (has
a push operation) or a push-down automaton, rather than other
solutions not using either of these two.

We have heard a principle that we want to name exercises by their story,
not by what they teach:
#1451 (comment)

The story here is about matching brackets, so I posit that that serves
as the name we want.

I have changed the major version number, as was done in the
retree->satellite rename, even though I do not feel strongly about
whether that should be necessary:
#1478

As we have gained experience in the rename of retree, we see that
problem-specifications is free to rename exercises at any point, without
waiting for all tracks to follow suit.
Of course, this operation should not be performed lightly since it
causes churn in the 31 (thirty-one) tracks implementing this exercise.

I rehash my prior allegations of
#693 below:

I contend that having the word `push` in the name unnecessarily biases
the solution space toward solutions that use a stack data structure (has
a *push* operation) or a *push*-down automaton, rather than other
solutions not using either of these two.

We have heard a principle that we want to name exercises by their story,
not by what they teach:
#1451 (comment)

The story here is about matching brackets, so I posit that that serves
as the name we want.

I have changed the major version number, as was done in the
retree->satellite rename, even though I do not feel strongly about
whether that should be necessary:
#1478

As we have gained experience in the rename of retree, we see that
problem-specifications is free to rename exercises at any point, without
waiting for all tracks to follow suit.
Of course, this operation should not be performed lightly since it
causes churn in the 31 (*thirty-one*) tracks implementing this exercise.
@petertseng
Copy link
Member Author

I have a specific question for @exercism/product : I am proposing here a rename of bracket-push. You can read its description at https://github.com/exercism/problem-specifications/blob/master/exercises/bracket-push/description.md . If you have input on what the name should be, I think it would be good to hear it.

I would prefer to avoid subjecting 31 tracks (the number of tracks implementing bracket-push) to having to rename bracket-push to name1, only to have to rename it to name2 later on. So if we can spend a bit of time to make sure we are reasonably happy with the new name, let's do.

@petertseng
Copy link
Member Author

petertseng commented Apr 5, 2019

Follow-up steps to execute this rename, if this proposal is accepted:

For your consideration, here are the 31 tracks that implement this exercise. Note that the first number you see is the difficulty. The second number you see is the depth of the exercise, defined as follows:

  • A core exercise's depth is its 0-based index within the ordered list of core exercises.
  • A side exercise's depth is equal to the depth of the core exercise that unlocks it.
  • A free-standing exercise's depth is always 0.
$ ruby implementing-tracks.rb bracket-push
              c  5 side  1  https://github.com/exercism/c/tree/master/exercises/bracket-push
         ceylon  6 core  6  https://github.com/exercism/ceylon/tree/master/exercises/bracket-push
        clojure  1 free  0  https://github.com/exercism/clojure/tree/master/exercises/bracket-push
            cpp  3 side  7  https://github.com/exercism/cpp/tree/master/exercises/bracket-push
        crystal  1 free  0  https://github.com/exercism/crystal/tree/master/exercises/bracket-push
         csharp  5 core 16  https://github.com/exercism/csharp/tree/master/exercises/bracket-push
           dart  3 free  0  https://github.com/exercism/dart/tree/master/exercises/bracket-push
         delphi  5 core 16  https://github.com/exercism/delphi/tree/master/exercises/bracket-push
         elixir  3 free  0  https://github.com/exercism/elixir/tree/master/exercises/bracket-push
            elm  7 free  0  https://github.com/exercism/elm/tree/master/exercises/bracket-push
         erlang  3 free  0  https://github.com/exercism/erlang/tree/master/exercises/bracket-push
         fsharp  7 side 11  https://github.com/exercism/fsharp/tree/master/exercises/bracket-push
             go  7 side 11  https://github.com/exercism/go/tree/master/exercises/bracket-push
        haskell  5 side  9  https://github.com/exercism/haskell/tree/master/exercises/bracket-push
           java  5 side 11  https://github.com/exercism/java/tree/master/exercises/bracket-push
     javascript  3 side  7  https://github.com/exercism/javascript/tree/master/exercises/bracket-push
         kotlin  5 side  9  https://github.com/exercism/kotlin/tree/master/exercises/bracket-push
            lua  5 side  4  https://github.com/exercism/lua/tree/master/exercises/bracket-push
            nim  1 side  3  https://github.com/exercism/nim/tree/master/exercises/bracket-push
    objective-c  5 free  0  https://github.com/exercism/objective-c/tree/master/exercises/bracket-push
          ocaml  4 free  0  https://github.com/exercism/ocaml/tree/master/exercises/bracket-push
pharo-smalltalk  4 side  2  https://github.com/exercism/pharo-smalltalk/tree/master/exercises/bracket-push
            php  1 free  0  https://github.com/exercism/php/tree/master/exercises/bracket-push
     purescript  1 free  0  https://github.com/exercism/purescript/tree/master/exercises/bracket-push
         python  1 side  6  https://github.com/exercism/python/tree/master/exercises/bracket-push
           ruby  7 side  8  https://github.com/exercism/ruby/tree/master/exercises/bracket-push
           rust  1 side  3  https://github.com/exercism/rust/tree/master/exercises/bracket-push
          scala  5 side  0  https://github.com/exercism/scala/tree/master/exercises/bracket-push
            sml  4 free  0  https://github.com/exercism/sml/tree/master/exercises/bracket-push
          swift  7 free  0  https://github.com/exercism/swift/tree/master/exercises/bracket-push
     typescript  5 free  0  https://github.com/exercism/typescript/tree/master/exercises/bracket-push

The script that made this is at https://github.com/petertseng/exercism-problem-specifications/blob/list-tracks/implementing-tracks.rb

Note that for tracks that have zero core exercises defined in their config.json, the first eleven exercises are treated as core exercises and the rest free (rules in https://github.com/exercism/website/blob/master/app/services/git/sync_track.rb). A few tracks have zero core exercises defined; out of these tracks, the single track that has core in eleventh or earlier position is ceylon, so it is core 6 instead of free 0 despite it being "core": false

@sshine
Copy link
Contributor

sshine commented Apr 5, 2019

There are only two hard things in Computer Science: cache invalidation and naming things.

I agree with your reasoning and am personally content with matching-brackets.

@ErikSchierboom
Copy link
Member

I agree with your reasoning and am personally content with matching-brackets.

Same here. @iHiD, what do you think?

@iHiD
Copy link
Member

iHiD commented Apr 12, 2019

Firstly, I'm grateful to @petertseng for such clear reasoning and the offers to do the legwork.
Secondly, My personal opinion is that this is a good change.
Thirdly: That's my personal opinion rather than my "exercism-leadership-role opinion". I'm happy to let y'all decide whatever you think is correct :)

As there's three 👍 and no 👎 after a week, I say go for it :)

@sshine
Copy link
Contributor

sshine commented Apr 12, 2019

I am unsure if we have a policy wrt. merge/rebase commits when there is only one commit in the PR.

@iHiD
Copy link
Member

iHiD commented Apr 12, 2019

I've never understood the point of merge commits and would appreciate being enlightened :)

@cmccandless
Copy link
Contributor

I've never understood the point of merge commits and would appreciate being enlightened :)

The only use I get out of them is that branches merged with merge commits will show up when git branch --merged is used; useful for cleaning up.

On my own personal projects, I tend to use merge commits when there is only 1 commit.

@SleeplessByte
Copy link
Member

I am unsure if we have a policy wrt. merge/rebase commits when there is only one commit in the PR.

I would squash here, for the simple reason that GH automagically adds the PR link to the title.

@petertseng
Copy link
Member Author

I've never understood the point of merge commits and would appreciate being enlightened :)

I'll describe three common situations we might see in PRs. Merge commits are only required in the very last case (last paragraph) I describe.

For this PR which was submitted as one commit and should end up as one commit, it does not seem particularly important, therefore I slightly favour using Squash and merge since the merge commit is not necessary.

For PRs which are submitted as multiple commits and should end up as one commit, Squash and merge is a necessity; a merge commit would leave commits in history that are not independently revertable or understandable, and may be in an intermediate state that do not pass tests (rendering the repo no longer bisectable).

For PRs which are submitted as multiple commits and should end up as multiple commits, Squash and merge is undesirable, since it creates a single commit that does multiple things, in violation of the principle that each commit should do one thing only. In this case, we must either use Create a merge commit or Rebase and merge. Rebase and merge has the disadvantage that we can no longer tell via git log what PR the commit came from (you can still tell via the GitHub web UI), so Create a merge commit preserves that.

@Stargator
Copy link
Contributor

@petertseng For your third example, a PR with multiple commits that should remain as multiple commits, I would suggest there is a way to tell after a Rebase and merge what is part of a single PR.

Have the PR referenced in each commit with the hastag identifier. Since it's a Rebase and Merge, it would all happen at once.

That's what my team at work does, we have the TaskID as part of the commit. In out case it's at the beginning, but for this repo it could be exercise: #ID Description

Then all the commits reference the exercise or section modified and what the work/change was.

@petertseng
Copy link
Member Author

Recalling that there are 31 tracks that would be affected, one may find it challenging that we don't have any hard numbers/rules on what constitutes sufficient quorum.

My personal take on it is: This repo holds a set of recommendations that are suitable for our language-agnostic specifications. Tracks are always free to deviate from them. For this PR, that means they can feel free to choose to ignore this rename and have it remain bracket-push on their track if they so wish. So for this repo, we just need to see sufficient (may require judgment) support and have addressed all objections (if any), and then we can be confident that it is the right recommendation to make. Then tracks can make of it what they will. I guess it's similar to the https://rave.apache.org/docs/governance/lazyConsensus.html idea.

Assuming the current state of nonzero support + zero objections remains, I plan to merge this PR when I actually have time to perform the additional steps (website-icons PR, issues on implementing tracks). I estimate in 48 hours.

@cmccandless
Copy link
Contributor

@SleeplessByte

I would squash here, for the simple reason that GH automagically adds the PR link to the title.

I don't know the answer to this off the top of my head for certain, so I'll put the question out there: does GitHub not put the PR link in the title for a merge commit?

Admittedly, the commits that get merged are not linked to the PR, but there should still be an actual merge commit that links to the PR.

@SleeplessByte
Copy link
Member

I don't know the answer to this off the top of my head for certain, so I'll put the question out there: does GitHub not put the PR link in the title for a merge commit?

Nope, unfortunately :)

@iHiD
Copy link
Member

iHiD commented Apr 13, 2019

Assuming the current state of nonzero support + zero objections remains, I plan to merge this PR when I actually have time to perform the additional steps (website-icons PR, issues on implementing tracks). I estimate in 48 hours.

You have my 👍 and gratitude on this plan :)

petertseng added a commit to petertseng/exercism-ceylon that referenced this pull request Apr 14, 2019
This rename was proposed in:
exercism/problem-specifications#1501

The rationale for the name is:

* to name the exercise by its story, not by what it potentially teaches
* to avoid unnecessarily biasing the solution space
petertseng added a commit to exercism/ceylon that referenced this pull request Apr 14, 2019
This rename was proposed in:
exercism/problem-specifications#1501

The rationale for the name is:

* to name the exercise by its story, not by what it potentially teaches
* to avoid unnecessarily biasing the solution space
sshine pushed a commit to exercism/haskell that referenced this pull request Apr 15, 2019
This rename was proposed in:
exercism/problem-specifications#1501

The rationale for the name is:

* to name the exercise by its story, not by what it potentially teaches
* to avoid unnecessarily biasing the solution space
SleeplessByte added a commit to SleeplessByte/ruby that referenced this pull request Apr 15, 2019
This rename was proposed in:
exercism/problem-specifications#1501

The rationale for the name is:

* to name the exercise by its story, not by what it potentially teaches
* to avoid unnecessarily biasing the solution space
@SleeplessByte
Copy link
Member

SleeplessByte commented Apr 15, 2019

@petertseng I took your list and added checkboxes. Checked them if a PR has been opened.

  • c
  • ceylon
  • clojure
  • cpp
  • crystal
  • csharp
  • dart
  • delphi
  • elixir
  • elm
  • erlang
  • fsharp
  • go
  • haskell
  • java
  • javascript
  • kotlin
  • lua
  • nim
  • objective-c
  • ocaml
  • pharo-smalltalk
  • php
  • purescript
  • python
  • ruby
  • rust
  • scala
  • sml
  • swift
  • typescript

SleeplessByte added a commit to SleeplessByte/csharp that referenced this pull request Apr 15, 2019
This rename was proposed in:
exercism/problem-specifications#1501

The rationale for the name is:

* to name the exercise by its story, not by what it potentially teaches
* to avoid unnecessarily biasing the solution space
@petertseng
Copy link
Member Author

I am quite late, and for this I apologise for missing my obligations, but I filed the issues. ruby implementing-tracks.rb bracket-push | wc -l says 20 tracks still have bracket-push, and you see I filed 18 issues. I didn't file the issue in Kotlin and Ruby tracks because PRs are already open. For all others, I did. Sorry I didn't use https://github.com/exercism/blazon to do it.

leenipper added a commit to leenipper/xgo that referenced this pull request May 12, 2019
Rename the bracket-push exercise to matching-brackets.

This rename was proposed in:
exercism/problem-specifications#1501

The rationale for the name is:

* to name the exercise by its story, not by what it potentially teaches
* to avoid unnecessarily biasing the solution space

Resolves exercism#1240
mikegehard pushed a commit to exercism/kotlin that referenced this pull request May 28, 2019
* bracket-push: rename to matching-brackets

This rename was proposed in:
exercism/problem-specifications#1501

The rationale for the name is:

* to name the exercise by its story, not by what it potentially teaches
* to avoid unnecessarily biasing the solution space

* 👀 rename reference in settings.gradle
edgerunner added a commit to edgerunner/elm that referenced this pull request Aug 27, 2019
petertseng added a commit to petertseng/exercism-ceylon that referenced this pull request Sep 1, 2019
petertseng added a commit to petertseng/exercism-ceylon that referenced this pull request Sep 1, 2019
sshine added a commit to exercism/sml that referenced this pull request Sep 3, 2019
This rename was proposed in exercism/problem-specifications#1501

The rationale for the name is:

* to name the exercise by its story, not by what it potentially teaches
* to avoid unnecessarily biasing the solution space
topstack1226 added a commit to topstack1226/typescript that referenced this pull request Jan 10, 2023
This rename was proposed in:
exercism/problem-specifications#1501

The rationale for the name is:

* to name the exercise by its story, not by what it potentially teaches
* to avoid unnecessarily biasing the solution space
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants