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

Improve debugability of error message for a malformed highlight tag #785

Merged
merged 1 commit into from Feb 27, 2013

Conversation

Projects
None yet
4 participants
@leonardehrenfried
Copy link
Contributor

leonardehrenfried commented Jan 29, 2013

I didn't add a test case since this change is tiny.

Is that ok or should I add a test that checks that the error message matches?

@mattr-

This comment has been minimized.

Copy link
Member

mattr- commented Jan 29, 2013

Did you run the tests and do they still pass after your change? If so,
then I see no reason to test the contents of the error message.

However, if the condition that causes this error in the first place is
not covered, then I think it's appropriate to add another pull request
for that. The lack of a test case for the actual error shouldn't keep
this from being merged.

@leonardehrenfried

This comment has been minimized.

Copy link
Contributor Author

leonardehrenfried commented Jan 29, 2013

I had one test failure before my commit

1) Failure: test: kramdown should convert quotes to smart quotes. (TestKramdown) [/Users/lenni/dev/jekyll/test/test_kramdown.rb:26]: <"<p>&ldquo;Pit&rsquo;hy&rdquo;</p>"> expected but was <"<p>&#8220;Pit&#8217;hy&#8221;</p>">.

and this one remains after the change. Since Travis runs the tests fine it seems that the failure is due to an outdated version of kramdown being installed on my machine by bundle install.

@mattr-

This comment has been minimized.

Copy link
Member

mattr- commented Jan 29, 2013

Ok. Thanks for confirming. The new error message also looks fine as well.

👍 from me.

@mattr-

This comment has been minimized.

Copy link
Member

mattr- commented Feb 26, 2013

@parkr or @mojombo any thoughts on this pull request?

@mattr-

This comment has been minimized.

Copy link
Member

mattr- commented Feb 26, 2013

@lenniboy you will most likely increase your chances of getting this merged if you put it on a feature branch and make sure it's up to date with the latest code in the master branch.

@parkr

This comment has been minimized.

Copy link
Member

parkr commented Feb 27, 2013

Looks good to me.

parkr added a commit that referenced this pull request Feb 27, 2013

Merge pull request #785 from lenniboy/master
Improve debugability of error message for a malformed highlight tag

@parkr parkr merged commit bd9a112 into jekyll:master Feb 27, 2013

1 check passed

default The Travis build passed
Details

parkr added a commit that referenced this pull request Feb 27, 2013

@jekyll jekyll locked and limited conversation to collaborators Feb 27, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.