markdown: add to track by bmkiefer · Pull Request #1060 · exercism/java

bmkiefer · 2017-11-29T23:07:38Z

I implemented the markdown exercise because the java track did not yet have it. I am unsure where I should put this in the curriculum. I picked after word search because markdown parsing seemed like a similar skill set, but I am open to change it.

Reviewer Resources:

Track Policies

bmkiefer · 2017-11-30T00:40:49Z

A couple things here. 1) I am not entirely sure why this recent build is failing. 2) I realize that I didn't fully read this README.md. This exercise is centered around refactoring a solution to be cleaner and still allow the tests to pass. Does anyone have ideas for how to make my optimal solution better and what things I could do to my solution to make it worse for learners to clean up. A forward and backward direction haha

sjwarner-bp · 2017-11-30T08:10:52Z

It seems like the Travis build is failing with the same issue it had in #1004 - I'll restart the build and see what happens! This is very odd, but both of these times have been when new exercises were added... Maybe this is a clue?

(Sorry for the request Frida - misclicked! 😱 )

sjwarner-bp · 2017-11-30T08:19:57Z

Hmm... seems to have failed again for the same reasons (those seen in #1004 too).

+echo ==================================================
==================================================
+echo 'Solving settings.gradle'
Solving settings.gradle
+echo ==================================================
==================================================
+./exercism --config .journey-test.exercism.json fetch java settings.gradle
127.0.0.1 - - [30/Nov/2017:08:15:43 +0000] "GET /v2/exercises/java/settings.gradle HTTP/1.1" 200 339 0.0072
Not Submitted:         1 problem
java (Settings.gradle) /home/travis/build/exercism/java/build/exercism/java/settings.gradle
New:                   1 problem
java (Settings.gradle) /home/travis/build/exercism/java/build/exercism/java/settings.gradle
unchanged: 0, updated: 0, new: 1
+cp -R -H '/home/travis/build/exercism/java/exercises/settings.gradle/.meta/src/reference/java/*' /home/travis/build/exercism/java/build/exercism/java/settings.gradle/src/main/java/
cp: cannot stat ‘/home/travis/build/exercism/java/exercises/settings.gradle/.meta/src/reference/java/*’: Not a directory

I don't think any of the team have a clear idea of why this happens (but I'd rather not just keep clicking the restart button)!

sjwarner-bp

Other than the mysterious case of the failing build, I think this PR overall is looking great!

One small suggestion that I would have is adding the topic refactoring in the config.json, as it is mentioned in the README.md. Feel free to disagree with me though - I'd be interested to hear what you think 🙂

Additionally, your starter implementation will need (very messy) code to perplex the users! 😆

FridaTveit

This is looking really good @bmkiefer :)

I've added some comments, mostly minor things about staying consistent with the code style we use on this track :)

The most important change is probably that as you remarked in your comment this exercise is about refactoring. So you should add a starter implementation with some really messy code that passes the tests :)

FridaTveit · 2017-11-30T09:19:19Z

+        "conditionals",
+        "pattern_matching"
+      ],
+      "unlocked_by": "word-search",


Exercises can only be unlocked by a core exercise (see https://github.com/exercism/docs/blob/master/language-tracks/configuration/exercises.md) so it can't be unlocked by word-search, sorry. word-search is unlocked by the core exercise scrabble-score so maybe this exercise could be unlocked by that as well? :)

FridaTveit · 2017-11-30T09:21:00Z

+class Markdown {
+
+    String parse(String markdown)
+    {


Please put the opening curly bracket on the line above:

String parse(String markdown) {

That goes for everywhere else in the file as well. That's the most common code style for Java and it would be good to be consistent in every exercise :)

FridaTveit · 2017-11-30T09:22:06Z

+        {
+            String lineResult = parseLine(lines[i]);
+
+            if(lineResult.matches(".*(<li>).*") && !activeList){


There should be a space before the opening curly bracket here :)

FridaTveit · 2017-11-30T09:22:40Z

+                result.append("</ul>");
+                result.append(lineResult);
+            }
+            else {


Please put the else on the line above to be consistent with the code style in other exercises :)

FridaTveit · 2017-11-30T09:24:40Z

+        return parse_(parse__(markdown));
+    }
+
+    private String parse_(String markdown)


Using underscores in method names can make them hard to read. I would recommend using words instead, e.g. parseUnderscore and parseDoubleUnderscore :)

FridaTveit · 2017-11-30T09:25:35Z

+        String input = "This will _be_ __mixed__";
+        String expected = "<p>This will <em>be</em> <strong>mixed</strong></p>";
+
+        assertEquals(expected, markdown.parse(input) );


There shouldn't be any whitespace before the last closing bracket :)

FridaTveit · 2017-11-30T10:19:21Z

@stkent @Smarticles101 any ideas why the build failed?

Smarticles101 · 2017-11-30T13:54:13Z

I'm going to push a change real quick. I think this could fix it this

Except then again it only happens sometimes, so I'm not sure why it's happening

sjwarner-bp · 2017-11-30T13:55:43Z

Hmm that does look very similar to what we're experiencing. Not sure why it only happens sometimes (unless it is to do with new exercises), but I'm excited to see if this works @Smarticles101 👍

sjwarner-bp · 2017-11-30T14:01:07Z

Still the same old...

+echo ==================================================
==================================================
+echo 'Solving settings.gradle'
Solving settings.gradle
+echo ==================================================
==================================================
+./exercism --config .journey-test.exercism.json fetch java settings.gradle
127.0.0.1 - - [30/Nov/2017:14:00:15 +0000] "GET /v2/exercises/java/settings.gradle HTTP/1.1" 200 339 0.0064
Not Submitted:         1 problem
java (Settings.gradle) /home/travis/build/exercism/java/build/exercism/java/settings.gradle
New:                   1 problem
java (Settings.gradle) /home/travis/build/exercism/java/build/exercism/java/settings.gradle
unchanged: 0, updated: 0, new: 1
+cp -R -H '/home/travis/build/exercism/java/exercises/settings.gradle/.meta/src/reference/java/*' /home/travis/build/exercism/java/build/exercism/java/settings.gradle/src/main/java/
cp: cannot stat ‘/home/travis/build/exercism/java/exercises/settings.gradle/.meta/src/reference/java/*’: Not a directory

😱

sjwarner-bp · 2017-11-30T14:08:32Z

And once more 😢

Smarticles101 · 2017-11-30T14:08:58Z

Not really sure why it's still failing... perhaps we can summon someone more familiar with bash

FridaTveit · 2017-11-30T14:14:16Z

It looks like it thinks settings.gradle is an exercise:

solve_all_exercises(exercism_exercises_dir="/home/travis/build/exercism/java/build/exercism", exercism_configfile=".journey-test.exercism.json", exercise_to_solve="settings.gradle")

Smarticles101 · 2017-11-30T14:16:31Z

Oh you're right @FridaTveit I see now

FridaTveit · 2017-11-30T14:21:31Z

Great, thanks @Smarticles101! Hopefully it'll work this time :)

Smarticles101 · 2017-11-30T14:26:34Z

It's going to work this time but for a different reason... It's hit GitHub's API rate limit lol #772

bmkiefer · 2017-11-30T18:22:40Z

Awesome feedback guys, thank you! Also, good to know why that build was breaking.

List of code issues that I added to the sample solution. let me know what you want to see or not.

removed some of code reuse
packed a lot of code into parse function
braces, spacing, and indentation is messed up in places
variable names and method names are unclear in places
using string concatenation in the main parse function instead of StringBuilder
remove break from header counter and added logic to for loop conditional
added unnecessary check to loops checking active lists

Making chaos was fun lol

Might be worth keeping these as notes for mentors somehow or maybe included as hints?

sjwarner-bp · 2017-11-30T18:26:16Z

Hi @bmkiefer - I am not sure what the others think about keeping track of this, but I think it could be a good idea to take that list and stick it into the .meta/hints.md of this exercise.

However, I am sure if this is not the ideal place to keep those notes then someone else might have a better idea! 🙂

FridaTveit

Nice! This is looking great!

As for where to put the hints, I definitely think it would be useful to have them somewhere. I don't think putting it in .meta/hints.md would be correct because I think that gets appended onto the readme. Is that right @stkent?

Also, the build is failing for the same reason as before, it appears we haven't quite managed to fix it yet. I think it would be a good idea to try and fix that in a separate PR to avoid adding too much unrelated noise to this PR. What do you think @Smarticles101, @sjwarner-bp, @stkent?

sjwarner-bp · 2017-11-30T19:00:49Z

Ah - I didn't know these got appended to README.md @FridaTveit - thanks for letting me know that 🙂 In that case I'd suggest that they should be stored somewhere else, but probably in that .meta hidden folder (that way it stays with this exercise)? Probably worth waiting until we decide where these are stored before merging 🙂

As for CI fixes, I'd definitely open an issue and start fixes in a new PR to avoid noise - ~~I'll open an issue now~~ Opened #1063 🙂

bmkiefer · 2017-11-30T19:47:26Z

Okay, what would you like me to do with that CI fix commit? I am confident this fixes the issue with the CI, but there may be a better place in the scripts to fix it, if anyone has an opinion. This bug occurs when the settings.gradle file changes, an exercise changes, and it doesnt hit the API limit on GitHub. If the API limit is hit, it doesn't refer to the change set when running the test suite, it just runs all the tests. It basically thinks that settings.gradle is another exercise and has to be ignored when its walking through exercises in order for the tests to complete correctly. It probably could be filtered out when fetching the exercises from Github and not when you are walking through them. I didn't like moving it into another PR because you almost need to new exercise implemented for bug to show itself because thats the only time that you are changing the settings.gradle file.

FridaTveit · 2017-11-30T20:02:09Z

Thanks for looking into that @bmkiefer :) I've been fixing that in a separate PR (#1064) since it doesn't relate to adding this new exercise. Just changing something in settings.gradle should cause the bug to show itself I think :) Feel free to review/suggest changes to that PR. I think all the CI changes that have been added to this PR should be reverted since they aren't related to this PR. It's always best to only deal with one issue in a PR. Otherwise it become hard to review and it makes the history messy.

Are you okay with that? Sorry for the confusion and adding lots of noise and additional problems to your PR! :)

bmkiefer · 2017-11-30T20:15:02Z

Yep, that is cool. I will revert the CI changes. Good teamwork!

sjwarner-bp · 2017-11-30T20:45:43Z

Great work everyone! That is one mystery fixed 🙂

In regards to this PR @bmkiefer, I've seen you've undone the CI changes! That's great - I think this whole thing is ready to merge when we have added your list about all the code issues you have written!

FridaTveit · 2017-11-30T20:48:31Z

The PR with the fix has been merged now so if you merge/rebase on master then the build should hopefully be okay (fingers crossed!) :)

bmkiefer · 2017-11-30T20:48:44Z

I need to undo the earlier ones yet and we will have to do a squash commit here as well when this is merged given the reverts will be a little messy.

FridaTveit · 2017-11-30T20:49:30Z

Agreed, squash and merge should tidy it up :)

bmkiefer · 2017-11-30T20:52:16Z

Okay, now all the CI work done here is reverted! Woo

trying something else

bmkiefer · 2017-11-30T21:00:36Z

Weird, it gave the commits new hashes after I rebased the change that @FridaTveit made on master.

FridaTveit

This looks great! Thanks for dealing with all of that @bmkiefer :)

And yes, rebasing does change the hashes :)

stkent · 2017-12-02T00:04:10Z

Great work, all!

stkent · 2017-12-02T00:05:57Z

As for where to put the hints, I definitely think it would be useful to have them somewhere. I don't think putting it in .meta/hints.md would be correct because I think that gets appended onto the readme. Is that right @stkent?

Agreed, notes for ~~mentors~~ maintainers should ideally live in the .meta directory (an exception: comments explaining why Java tests deviate from the canonical tests currently live inline in the test suites.)

bmkiefer · 2017-12-02T00:28:23Z

Given the notes that I previously wrote only helps the mentors of track, do we even want to keep them in the repo? The problem specification somewhat outlines that the learners should be identifying the changes that they made to the code. Either way, It's worth setting a precedent given there is a couple other refactoring exercises that haven't been implemented. If we want to forego using them, then we can merge this.

FridaTveit · 2017-12-04T12:07:26Z

I agree that since these are really notes for mentors, it doesn't really make sense for them to be kept in the repo. So I'm happy to merge this as is :)

What do you think @exercism/java?

sjwarner-bp · 2017-12-04T14:35:38Z

If that is what you think @FridaTveit I am happy to agree!

stkent · 2017-12-05T00:10:24Z

+1 we should figure out where to put mentoring resources (or wait for nextercism to drive a discussion along those lines) before including them.

bmkiefer changed the title ~~add markdown exercise to track~~ markdown: add to track Nov 29, 2017

sjwarner-bp requested review from FridaTveit and removed request for FridaTveit November 30, 2017 08:06

sjwarner-bp suggested changes Nov 30, 2017

View reviewed changes

FridaTveit suggested changes Nov 30, 2017

View reviewed changes

FridaTveit approved these changes Nov 30, 2017

View reviewed changes

sjwarner-bp mentioned this pull request Nov 30, 2017

Travis CI build intermittently failing #1063

Closed

bmkiefer force-pushed the markdown-add-to-track branch from 891a8f8 to 77c6a55 Compare November 30, 2017 19:05

bmkiefer force-pushed the markdown-add-to-track branch from 7b39709 to 77c6a55 Compare November 30, 2017 20:26

bmkiefer and others added 9 commits November 30, 2017 14:54

add markdown exercise to track

1c68b52

fix uuid for markdown

284806b

Update journey-test.sh

c256bed

Update journey-test.sh

f203e68

trying something else

Update run-journey-test-from-ci.sh

7d3c979

update markdown ref solution and add initial solution

9f05774

revert b3220e5

f42a658

revert 82aa9f9

f9576bd

revert cd5f0cb

ac9cb0e

bmkiefer force-pushed the markdown-add-to-track branch from e7fe054 to ac9cb0e Compare November 30, 2017 20:57

FridaTveit approved these changes Nov 30, 2017

View reviewed changes

sjwarner-bp approved these changes Dec 4, 2017

View reviewed changes

stkent merged commit 18d82f2 into exercism:master Dec 5, 2017

sjwarner-bp mentioned this pull request Dec 20, 2017

rail-fence-cipher: add to track #765

Merged

Uh oh!

Conversation

bmkiefer commented Nov 29, 2017

Uh oh!

bmkiefer commented Nov 30, 2017

Uh oh!

sjwarner-bp commented Nov 30, 2017

Uh oh!

sjwarner-bp commented Nov 30, 2017

Uh oh!

sjwarner-bp 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!

FridaTveit left a comment

Choose a reason for hiding this comment

Uh oh!

FridaTveit Nov 30, 2017

Choose a reason for hiding this comment

Uh oh!

FridaTveit Nov 30, 2017

Choose a reason for hiding this comment

Uh oh!

FridaTveit Nov 30, 2017

Choose a reason for hiding this comment

Uh oh!

FridaTveit Nov 30, 2017

Choose a reason for hiding this comment

Uh oh!

FridaTveit Nov 30, 2017

Choose a reason for hiding this comment

Uh oh!

FridaTveit Nov 30, 2017

Choose a reason for hiding this comment

Uh oh!

FridaTveit commented Nov 30, 2017

Uh oh!

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

Uh oh!

Uh oh!

sjwarner-bp commented Nov 30, 2017

Uh oh!

sjwarner-bp commented Nov 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sjwarner-bp commented Nov 30, 2017

Uh oh!

Smarticles101 commented Nov 30, 2017

Uh oh!

FridaTveit commented Nov 30, 2017

Uh oh!

Smarticles101 commented Nov 30, 2017

Uh oh!

FridaTveit commented Nov 30, 2017

Uh oh!

Smarticles101 commented Nov 30, 2017

Uh oh!

bmkiefer commented Nov 30, 2017

Uh oh!

sjwarner-bp commented Nov 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

FridaTveit left a comment

Choose a reason for hiding this comment

Uh oh!

sjwarner-bp commented Nov 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

FridaTveit commented Nov 30, 2017

Uh oh!

bmkiefer commented Nov 30, 2017

Uh oh!

sjwarner-bp commented Nov 30, 2017

Uh oh!

FridaTveit commented Nov 30, 2017

Uh oh!

bmkiefer commented Nov 30, 2017

sjwarner-bp left a comment •

edited

Loading

Smarticles101 commented Nov 30, 2017 •

edited

Loading

sjwarner-bp commented Nov 30, 2017 •

edited

Loading

sjwarner-bp commented Nov 30, 2017 •

edited

Loading

sjwarner-bp commented Nov 30, 2017 •

edited

Loading

bmkiefer commented Nov 30, 2017 •

edited

Loading

bmkiefer commented Nov 30, 2017 •

edited

Loading

stkent commented Dec 2, 2017 •

edited

Loading

bmkiefer commented Dec 2, 2017 •

edited

Loading