[WIP] Implement exercise run-length-encoding #862

MatForsberg · 2017-09-17T21:51:19Z

No description provided.

1.9 was released on 24 August 2017: https://golang.org/doc/go1.9 1.8.3 was released on 24 May 2017: https://golang.org/doc/devel/release.html#go1.8.minor This keeps up with the policy of only testing the current & previous release.

This describes the bonus test and how to run it. closes #832

tleen · 2017-09-17T22:41:20Z

@MatForsberg I see nothing in changed files?

Closes #863

MatForsberg · 2017-09-18T21:31:14Z

@tleen I apologize. I am new at this. I was doing my best to follow the directions on https://github.com/exercism/docs/blob/master/you-can-help/implement-an-exercise-from-specification.md . Specifically "Avoiding Duplicate Work

When you decide to implement an exercise, first check that there are no open pull requests for the same exercise.

Then open a "work in progress" (WIP) pull request, so others will see that you're working on it.

The way to open a WIP pull request even if you haven't done any work yet is:

Fork and clone the repository.
Check out a branch for the exercise.
Add an empty commit git commit --allow-empty -m "Implement exercise <slug>" (replace with the actual name of the exercise).
Push the new branch to your repository, and open a pull request against that branch.

Once you have added the actual exercise, then you can rebase your branch onto the upstream master, which will make the WIP commit go away."

I have finished the exercise but having a little trouble with the test generator since the test data has sub cases.

tleen · 2017-09-18T22:15:49Z

Ah @MatForsberg I see what you are doing now thanks. Conventionally we have been prefixing WIPs as "[WIP]". I'll mod the title now so we don't get confused.

We are requesting that anything bug bugfixes go into the nextercism branch. The changes in #864 contain an explanation of why we are doing that right now. Basically there is a v2 of Exercism coming out with a different meta-structure for exercise organization and we are making major changes with that in mind. It should not impact how you implement the exercise, just where it should be eventually merged into the track.

Thanks for working on this one! 🎉

leenipper

Thanks @MatForsberg for this work.

The 'go test' ran locally for me, so I'm guessing that the formatting is throwing off the travis-ci test.

So, simply do:
go fmt cases_test.go
go fmt example.go
go fmt run_length_encoding_test.go

push up the additional commit for the formatting change, and perhaps the travis-ci will pass at that point. Then we can pick it up again from there.

Formatted go files and added UUID to config.json

MatForsberg · 2017-09-19T11:06:08Z

I think the problem is i am missing the uuid for this exercise. I generated one online and added it to the config.json

robphoenix · 2017-09-19T14:20:12Z

Thanks for this @MatForsberg and Welcome! 🎉

I have finished the exercise but having a little trouble with the test generator since the test data has sub cases.

If you like we could split this into two PR's, this one to just implement the exercise, and another to implement the test generator.

As @tleen mentioned Exercism is midway through a significant change, and we've got two main branches we're working with in this repo. Unfortunately we hadn't updated the documentation for this before your PR: https://github.com/exercism/go/blob/master/.github/PULL_REQUEST_TEMPLATE.md

I'm going to change the base branch of this PR from master to nextercism which is going to mean a few things will change, specifically the config.json file will need to be updated to the new structure: https://github.com/exercism/go/blob/nextercism/config.json and we'll have to consider where in the exercise structure to place this exercise. This is the first new exercise for the restructure, so for now I'd put it in the config.json as follows:

{
      "core": false,
      "difficulty": 4,
      "slug": "run-length-encoding",
      "topics": [
       "strings",
        "transforming",
        "parsing"
      ],
      "unlocked_by": null,
      "uuid": "29ea064e-9d2a-11e7-abc4-cec278b6b50a"
},

towards the end between the bowling and binary exercises.

Thanks again, and sorry for the complications.

MatForsberg · 2017-09-19T21:36:09Z

@robphoenix If i understand you right i made the necessary change. I wrote over the change.json that i had with the one from the link and then put the json you suggested in between bowling and binary. Thank you all for your help.

leenipper

Maybe a rebase on the nextercism branch and a force push can clear up the confusion in the change set regarding exercise robot-name files.

Other than that, dropping the stub file is the only needed change that I see.

leenipper · 2017-09-30T03:35:58Z

exercises/run-length-encoding/run_length_encoding.go

@@ -0,0 +1,5 @@
+package run_length_encoding


I thought that a stub won't be needed for run-length-encoding, since raindrops is likely before this one and raindrops is the first one without a stub. Is that correct @robphoenix and @tleen ?

Not really sure yet where this will land in nextercism. But it won't be in core and probably after raindrops. It's not like we can't have stubs after that, but we may not want to. We can modify this exercise afterwards when we place it?

I think it's best to remove the stub file. I don't think this exercise will be before raindrops and we can see when v2 launches if we need to add any stubs to later exercises.

leenipper · 2017-09-30T03:37:19Z

exercises/robot-name/bonus_example.go

 	"math/rand"
 )

+const testVersion = 1


Not sure why the change to this robot-name exercise is here. Base branch isn't quite right yet maybe?

leenipper · 2017-09-30T03:38:30Z

exercises/robot-name/.meta/hints.md

@@ -0,0 +1,13 @@
+### Bonus exercise


robot-name exercise change ?

tleen · 2017-10-01T02:59:35Z

.travis.yml

-  - 1.7.5
-  - 1.8
+  - 1.8.3
+  - 1.9


I think we have picked up some weirdness in the branch here. Perhaps a rebase to nextercism is in order? config especially? These travis changes shouldn't be here. Git can be tough sometimes!

Formatted go files and added UUID to config.json

This reverts commit f9c6ef1, reversing changes made to 27467bf.

… into run-length-encoding

MatForsberg · 2017-10-01T19:05:17Z

reverted commit with the robot and travis changes

leenipper

Thanks @MatForsberg for working to try and get it straight. The content for the new exercise looks good; but the merge cannot happen as it is right now.

Don't feel bad; git is a tough nut sometimes, and all of us have had our difficulties. (And I still do at times!)

IMO, it may be easiest to resolve by using a new PR, whose branch is based on nextercism. What to you think @tleen and @robphoenix ?

Since all files but one (config.json change being the exception) are new, you could start a new branch based off upstream/nextercism, add the new files into it, and add the config.json change, and I think the changes would be clean at that point.

I know the original base branch was master instead of nextercism, and I'm not github savvy enough to know how changing the base branch works "on-the-fly" so to speak.

We need some consensus before moving forward on it.

leenipper · 2017-10-01T22:00:13Z

.github/PULL_REQUEST_TEMPLATE.md

@@ -0,0 +1,23 @@
+## Exercism v2


This file is on the master branch, but not on the nextercism branch.

I fetched your repo and checked out the branch; sure enough it has a lot of testVersion references, which have been removed from nextercism branch. That says your run-length-encoding branch isn't a fresh rebase on nextercism.

tleen · 2017-10-02T13:02:20Z

I do think that is a better approach. I got lost in the git weeds the same way too over the weekend doing a much simpler change (see #877). I ended up closing the PR and redoing it again from a clean checkout of the repo. Git is cool when it's cool, but it can get out of hand real fast!

robphoenix · 2017-10-02T13:54:51Z

Hi @MatForsberg

I apologize. I am new at this.

Firstly, please, no need to apologise and be rest assured these difficulties we're dealing with trying to get this PR merged are not your fault and are ones we have all encountered, and honestly still do, as @tleen mentions. I hope this doesn't overshadow your new experience contributing to open source! And on that note, thankyou for taking the time and effort to open this PR.

So, resolving the commit log to be able to merge this in...
There are two approaches that I can see; start again with a new PR, or do an interactive rebase.
The first is going to be the easiest, and as this is contributing a new exercise, it shouldn't be too traumatic. As @leenipper notes:

Since all files but one (config.json change being the exception) are new, you could start a new branch based off upstream/nextercism, add the new files into it, and add the config.json change, and I think the changes would be clean at that point.

Otherwise I able to create a clean fork of this PR: nextercism...robphoenix:pr-862 using an interactive rebase. This will enable you to drop other peoples commits and squash yours into a single one.

We want to amend the last 15 commits of this branch so run the following command in your terminal within your branch for this PR:

git rebase -i HEAD~15

This will open up your editor and list the last 15 commits, excluding merge commits, and look something like the following:

   1 pick fe857c0 roman-numerals: remove non-canonical test data (#845)                                                                                     
   2 pick 66fb926 README: Change exercise-slug to fizzbuzz in examples (#849)                                                                               
   3 pick d7a3499 robot-simulator: Update grammar and wording (#851)                                                                                        
   4 pick 0243916 gen/gen.go: correct dirExercise determination (#850)                                                                                      
   5 pick 60b1999 Perfect Numbers: added benchmark test for Classify (#854)                                                                                 
   6 pick 405cd76 bob: replace drivel with remark (#827)                                                                                                    
   7 pick 1e0d6fa [nextercism] update config.json (#831)                                                                                                    
   8 pick 27467bf [nextercism] Restructure config.json (#835)                                                                                               
   9 pick a1d1182 travis: upgrade Go version (#855)                                                                                                         
  10 pick aa45461 robot-name: Add hints.md file (#858)                                                                                                      
  11 pick c33490d Add PR template (#864)                                                                                                                    
  12 pick c23e1c1 added run-length-encoding excercise                                                                                                       
  13 pick 5571312 included run-length-encoding in config.json                                                                                               
  14 pick 38a5c26 Formatted and added UUID                                                                                                                  
  15 pick 419ee59 Changed config.json configuration                                                                                                         
  16 pick 1fbf5aa Revert "Merge remote-tracking branch 'exercism/master' into nextercism"                                                                   
  17 # pick 0a34c66832e0bdfd2431718126473f4d8eafe4a2 Implement exercise run-length-encoding                                                                 
  18 pick 31e6092 added run-length-encoding excercise                                                                                                       
  19 pick 512719e included run-length-encoding in config.json                                                                                               
  20 pick a85ba41 Formatted and added UUID                                                                                                                  
  21 pick 89ec9c8 Changed config.json configuration                                                                                                         
  22                                                                                                                                                        
  23 # Rebase 37ee18e..1c8ff94 onto 37ee18e (20 commands)                                                                                                   
  24 #                                                                                                                                                      
  25 # Commands:                                                                                                                                            
  26 # p, pick = use commit                                                                                                                                 
  27 # r, reword = use commit, but edit the commit message                                                                                                  
  28 # e, edit = use commit, but stop for amending                                                                                                          
  29 # s, squash = use commit, but meld into previous commit                                                                                                
  30 # f, fixup = like "squash", but discard this commit's log message                                                                                      
  31 # x, exec = run command (the rest of the line) using shell                                                                                             
  32 # d, drop = remove commit                                                                                                                              
  33 #                                                                                                                                                      
  34 # These lines can be re-ordered; they are executed from top to bottom.                                                                                 
  35 #                                                                                                                                                      
  36 # If you remove a line here THAT COMMIT WILL BE LOST.                                                                                                  
  37 #                                                                                                                                                      
  38 # However, if you remove everything, the rebase will be aborted.                                                                                       
  39 #                                                                                                                                                      
  40 # Note that empty commits are commented out

I did this in two steps, first dropping unwanted commits and then squashing your commits into one.
To drop the unnecessary commits we can change this list to the following:

   1 pick fe857c0 roman-numerals: remove non-canonical test data (#845)                                                                                     
   2 pick 66fb926 README: Change exercise-slug to fizzbuzz in examples (#849)                                                                               
   3 pick d7a3499 robot-simulator: Update grammar and wording (#851)                                                                                        
   4 pick 0243916 gen/gen.go: correct dirExercise determination (#850)                                                                                      
   5 pick 60b1999 Perfect Numbers: added benchmark test for Classify (#854)                                                                                 
   6 pick 405cd76 bob: replace drivel with remark (#827)                                                                                                    
   7 pick 1e0d6fa [nextercism] update config.json (#831)                                                                                                    
   8 pick 27467bf [nextercism] Restructure config.json (#835)                                                                                               
   9 d a1d1182 travis: upgrade Go version (#855)                                                                                                            
  10 d aa45461 robot-name: Add hints.md file (#858)                                                                                                         
  11 d c33490d Add PR template (#864)                                                                                                                       
  12 pick c23e1c1 added run-length-encoding excercise                                                                                                       
  13 pick 5571312 included run-length-encoding in config.json                                                                                               
  14 pick 38a5c26 Formatted and added UUID                                                                                                                  
  15 pick 419ee59 Changed config.json configuration                                                                                                         
  16 pick 1fbf5aa Revert "Merge remote-tracking branch 'exercism/master' into nextercism"                                                                   
  17 d 0a34c66832e0bdfd2431718126473f4d8eafe4a2 Implement exercise run-length-encoding                                                                      
  18 pick 31e6092 added run-length-encoding excercise                                                                                                       
  19 pick 512719e included run-length-encoding in config.json                                                                                               
  20 pick a85ba41 Formatted and added UUID                                                                                                                  
  21 pick 89ec9c8 Changed config.json configuration

Notice the lines where pick has been changed to d, including line 17 the initial empty commit.
If we then save the file and exit we'll be in the rebase, with this message:

The previous cherry-pick is now empty, possibly due to conflict resolution.
If you wish to commit it anyway, use:

    git commit --allow-empty

Otherwise, please use 'git reset'
interactive rebase in progress; onto 37ee18e
Last commands done (16 commands done):
   pick 419ee59 Changed config.json configuration
   pick 1fbf5aa Revert "Merge remote-tracking branch 'exercism/master' into nextercism"
Next commands to do (5 remaining commands):
   d 0a34c66 Implement exercise run-length-encoding
   pick 31e6092 added run-length-encoding excercise
You are currently rebasing branch 'pr-862-example' on '37ee18e'.

nothing to commit, working tree clean
Could not apply 1fbf5aa... Revert "Merge remote-tracking branch 'exercism/master' into nextercism"

I did a git rebase --continue here to move on, where I had to resolve some conflicts:

Auto-merging exercises/run-length-encoding/run_length_encoding_test.go
CONFLICT (add/add): Merge conflict in exercises/run-length-encoding/run_length_encoding_test.go
Auto-merging exercises/run-length-encoding/example.go
CONFLICT (add/add): Merge conflict in exercises/run-length-encoding/example.go
Auto-merging exercises/run-length-encoding/cases_test.go
CONFLICT (add/add): Merge conflict in exercises/run-length-encoding/cases_test.go
error: could not apply 31e6092... added run-length-encoding excercise

When you have resolved this problem, run "git rebase --continue".
If you prefer to skip this patch, run "git rebase --skip" instead.
To check out the original branch and stop rebasing, run "git rebase --abort".

Could not apply 31e6092... added run-length-encoding excercise

After fixing the conflicts I ran a git add --all and git rebase --continue again to complete the rebase, removing these commits.
So now we have these:

Mat Forsberg              13 days ago          Changed config.json configuration                                      bde579e
 Mat Forsberg              13 days ago          Formatted and added UUID                                               33aef55
 Mat Forsberg              2 weeks ago          included run-length-encoding in config.json                            6963d40
 Mat Forsberg              13 days ago          Changed config.json configuration                                      2dd6b0c
 Mat Forsberg              13 days ago          Formatted and added UUID                                               1100308
 Mat Forsberg              2 weeks ago          included run-length-encoding in config.json                            771e485
 Mat Forsberg              2 weeks ago          added run-length-encoding excercise                                    c4397a1

If we run git rebase -i HEAD~7 then we can change the word pick to fixup or squash and merge all these commits into one.

   1 pick c4397a1 added run-length-encoding excercise                                                                                                       
   2 f 771e485 included run-length-encoding in config.json                                                                                                  
   3 f 1100308 Formatted and added UUID                                                                                                                     
   4 f 2dd6b0c Changed config.json configuration                                                                                                            
   5 f 6963d40 included run-length-encoding in config.json                                                                                                  
   6 f 33aef55 Formatted and added UUID                                                                                                                     
   7 f bde579e Changed config.json configuration

This is a ridiculously long comment, and I don't mind if you don't read it and just open a new PR! 🤣 tbh I just wanted to see if this could be resolved with an interactive rebase! Since I learned about interactive rebase I do it all the time to tidy up my messy git history! I have an alias for it in my global gitconfig - https://github.com/robphoenix/dotfiles/blob/master/gitconfig#L43

robphoenix · 2017-10-02T14:55:52Z

exercises/run-length-encoding/cases_test.go

@@ -0,0 +1,32 @@
+package run_length_encoding


While we don't have a generator for this exercise, it would be better to keep the test cases in the run_length_encoding_test.go file. We can change this when a test generator is added.

robphoenix · 2017-10-02T14:56:52Z

exercises/run-length-encoding/example.go

@@ -0,0 +1,55 @@
+package run_length_encoding


the package name should be changed. https://blog.golang.org/package-names
Maybe encode ?
What do you think @tleen @leenipper

Yep something more Go-ish.

robphoenix · 2017-10-02T15:04:52Z

exercises/run-length-encoding/example.go

+	return output
+}
+
+func getEncodedOutput(count *int, i int, s string, output *string) {


As noted here, I'd change this function name. Maybe encode?

I think this function would work better if it returned the updated variables, rather than altering pointers;

func encode(count int, i int, s string, output string) (int, string) { if count > 1 { output += fmt.Sprintf("%d%c", count, s[i-1]) return 1, output } output += fmt.Sprintf("%c", s[i-1]) return 1, output }

you could then use it like so:

count, output = encode(count, i, s, output)

It's more explicit about what it's doing, and simplifies the function itself.

robphoenix · 2017-10-02T20:15:13Z

exercises/run-length-encoding/example.go

+
+func RunLengthEncode(s string) string {
+	count := 1
+	output := ""


It's more idiomatic to write var output string if you're going to initialise a variable to it's zero value

MatForsberg · 2017-10-02T20:19:22Z

Please do not delete i am working on incorporating changes

robphoenix · 2017-10-02T20:20:26Z

exercises/run-length-encoding/example.go

+func RunLengthDecode(s string) string {
+	count := 1
+	stringCount := ""
+	output := ""


these two lines could be simplified:

var stringCount, output string

robphoenix · 2017-10-02T20:21:51Z

No worries @MatForsberg, I've left a few comments.

robphoenix and others added 3 commits September 12, 2017 12:40

travis: upgrade Go version (#855)

a1d1182

1.9 was released on 24 August 2017: https://golang.org/doc/go1.9 1.8.3 was released on 24 May 2017: https://golang.org/doc/devel/release.html#go1.8.minor This keeps up with the policy of only testing the current & previous release.

robot-name: Add hints.md file (#858)

aa45461

This describes the bonus test and how to run it. closes #832

Implement exercise run-length-encoding

0a34c66

This was referenced Sep 18, 2017

[nextercism] Remove all exercise test versioning #859

Closed

Create PR Template for nextercism #863

Closed

Add PR template (#864)

c33490d

Closes #863

Mat Forsberg added 2 commits September 18, 2017 16:41

added run-length-encoding excercise

31e6092

included run-length-encoding in config.json

512719e

tleen changed the title ~~Implement exercise run-length-encoding~~ [WIP] Implement exercise run-length-encoding Sep 18, 2017

leenipper reviewed Sep 19, 2017

View reviewed changes

Formatted and added UUID

a85ba41

Formatted go files and added UUID to config.json

robphoenix changed the base branch from master to nextercism September 19, 2017 14:20

Changed config.json configuration

89ec9c8

robphoenix added the Nextercism label Sep 21, 2017

Merge remote-tracking branch 'exercism/master' into nextercism

f9c6ef1

leenipper suggested changes Sep 30, 2017

View reviewed changes

tleen reviewed Oct 1, 2017

View reviewed changes

Mat Forsberg and others added 6 commits October 1, 2017 13:37

added run-length-encoding excercise

c23e1c1

included run-length-encoding in config.json

5571312

Formatted and added UUID

38a5c26

Formatted go files and added UUID to config.json

Changed config.json configuration

419ee59

Revert "Merge remote-tracking branch 'exercism/master' into nextercism"

1fbf5aa

This reverts commit f9c6ef1, reversing changes made to 27467bf.

Merge branch 'run-length-encoding' of https://github.com/MatForsberg/go…

1c8ff94

… into run-length-encoding

leenipper reviewed Oct 1, 2017

View reviewed changes

robphoenix suggested changes Oct 2, 2017

View reviewed changes

robphoenix reviewed Oct 2, 2017

View reviewed changes

MatForsberg closed this Oct 2, 2017

robphoenix reviewed Oct 2, 2017

View reviewed changes

MatForsberg deleted the run-length-encoding branch October 2, 2017 20:22

MatForsberg mentioned this pull request Oct 3, 2017

[WIP] Adding run length encoding exercise #879

Merged

tleen mentioned this pull request Oct 6, 2017

travis: update go versions #887

Merged

petertseng mentioned this pull request Oct 31, 2017

implement-an-exercise-from-specification: Remove avoiding duplicate work section. exercism/legacy-docs#108

Merged

Uh oh!

[WIP] Implement exercise run-length-encoding #862

[WIP] Implement exercise run-length-encoding #862

Uh oh!

Conversation

MatForsberg commented Sep 17, 2017

Uh oh!

tleen commented Sep 17, 2017

Uh oh!

MatForsberg commented Sep 18, 2017

Uh oh!

tleen commented Sep 18, 2017

Uh oh!

leenipper left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MatForsberg commented Sep 19, 2017

Uh oh!

robphoenix commented Sep 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MatForsberg commented Sep 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

leenipper left a comment

Choose a reason for hiding this comment

Uh oh!

leenipper Sep 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MatForsberg commented Oct 1, 2017

Uh oh!

leenipper left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tleen commented Oct 2, 2017

Uh oh!

robphoenix commented Oct 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robphoenix Oct 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MatForsberg commented Oct 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robphoenix commented Oct 2, 2017

Uh oh!

Uh oh!

leenipper left a comment •

edited

Loading

robphoenix commented Sep 19, 2017 •

edited

Loading

MatForsberg commented Sep 19, 2017 •

edited

Loading

leenipper Sep 30, 2017 •

edited

Loading

robphoenix commented Oct 2, 2017 •

edited

Loading

robphoenix Oct 2, 2017 •

edited

Loading