-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Implement exercise markdown #797
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
Conversation
Done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your work!
I've left a couple of minor comments, please take a look
"slug": "markdown", | ||
"difficulty": 3, | ||
"topics": [ | ||
"refactoring" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure that this topic is appropriate, could you please explain your reasoning?
You can use this list of general topics common among all tracks - https://github.com/exercism/problem-specifications/blob/master/TOPICS.txt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
regular_expressions
, pattern_matching
are good candidates for example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look at this file:
"The markdown exercise is a refactoring exercise. There is code that parses a given string with Markdown syntax and returns the associated HTML for that string. Even though this code is confusingly written and hard to follow, somehow it works and all the tests are passing! Your challenge is to re-write this code to make it easier to read and maintain while still making sure that all the tests keep passing."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@m-a-ge, this exercise is intended to be a refactoring exercise rather than an implementation one (see the README). This also explains the presence of a working solution (does TravisCI currently test this solution? I feel like it needs to) in markdown.py
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@paulglass sorry, I've missed it due to amount of opened PRs 😕
@N-Parsons thanks!
No changes needed then 😄
|
||
|
||
def parse_markdown(markdown): | ||
lines = markdown.split('\n') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you forgot to replace an actual solution with an exercise placeholder for the learner.
Could you please fix this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Learner should refactor this code as I mentioned before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@paulglass yeah, I see it now, thanks
import unittest | ||
from markdown import parse_markdown | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please also leave a comment stating what version of canonical-data.json
the tests were adopted as discussed in #784?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@paulglass, the new format is:
# Tests adapted from `problem-specifications//canonical-data.json` @ v1.0.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
Thanks a lot for adding such an interesting exercise, @paulglass! |
resolves #729
Want to implement this