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

markdown: Extra tests to ensure markdown symbols are not incorrectly interpreted #1291

Merged
merged 2 commits into from Aug 9, 2018

Conversation

Projects
None yet
5 participants
@grantwinney
Contributor

grantwinney commented Aug 8, 2018

I recently reviewed a nicely refactored solution for the markdown exercise, but I noticed that while the original code passes for the tests I'm suggesting, the student's newly refactored code did not. It's not the student's fault, since the existing tests all passed, but I think these additional ones may be helpful.

I suggested these tests to the student, who was able to rework the code in a second iteration and make them pass.

My reasoning was that it's entirely reasonable to have a header where the text has a hash in it (like # My C# Header and that shouldn't be interpreted as an H2 header.

What does everyone think? If it's too much, or gets away from the goals of this exercise too much, I'm fine with leaving them out.

@rpottsoh

These look like nice improvements to me. Thanks @grantwinney.

Since the uptick in mentoring with v2 coming online, there has been an increase in the frequency of PRs like this. 👍

I am going to let this sit for now so others may have an opportunity to comment.

@petertseng

This comment has been minimized.

Member

petertseng commented Aug 8, 2018

If I may ask a question about the *** contained in these new cases

There are no cases that show that *** works standalone, so a parser that doesn't handle *** at all doesn't have to do any extra handling here. The only extra handling required is that of #.

So, I think it makes the most sense if either one of the two happens:

  • Adding a case where *** converts to an <hr/> (I think that's what it converts to)
  • Removing *** from these new cases
@grantwinney

This comment has been minimized.

Contributor

grantwinney commented Aug 8, 2018

@petertseng Hm, maybe having three asterisks is confusing since that is valid markdown, and it's not what I intended to test. What I was trying to make sure was that a # or * in the middle of the text wasn't inadvertently interpreted as a header or list item.

Maybe it'd be more to-the-point if I had used a single asterisk?

{
    "description": "with markdown symbols in the header text that should not be interpreted",
    "property": "parse",
    "input": {
        "markdown": "# This is a header with # and * in the text"
    },
    "expected": "<h1>This is a header with # and * in the text</h1>"
},
{
    "description": "with markdown symbols in the list item text that should not be interpreted",
    "property": "parse",
    "input": {
        "markdown": "* Item 1 with a # in the text\n* Item 2 with * in the text"
    },
    "expected": "<ul><li>Item 1 with a # in the text</li><li>Item 2 with * in the text</li></ul>"
},
{
    "description": "with markdown symbols in the paragraph text that should not be interpreted",
    "property": "parse",
    "input": {
        "markdown": "This is a paragraph with # and * in the text"
    },
    "expected": "<p>This is a paragraph with # and * in the text</p>"
}
@petertseng

This comment has been minimized.

Member

petertseng commented Aug 8, 2018

Indeed, if the intent to test that it's not a list item, I think a single asterisk achieves the goal better.

@ErikSchierboom

Thanks for doing this PR!

@rpottsoh rpottsoh merged commit 44dc5b2 into exercism:master Aug 9, 2018

2 checks passed

WIP ready for review
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment