Handle nil language in highlighter #53
Conversation
|
Some feedback on the tests: I'm not a huge fan of this level of reliance on mocks. I feel like they've described the exact implementation and not the expected behavior, which makes them particularly brittle to refactoring. Ideally the way to test this is to create a middleman site instance, add this TOC mixin, and then call methods on the instance and verify their output. Would you mind taking a few more hours to investigate if this is possible? I'd feel much more confident in our tests moving forward under a setup like that. Thanks! |
tundal45
force-pushed
the
handle-nil-language-in-highlighter
branch
from
4b7c226
to
f88eaa5
Compare
|
@trek As we discussed, the tests now primarily mock out |
tundal45
force-pushed
the
handle-nil-language-in-highlighter
branch
from
f88eaa5
to
2f6e775
Compare
|
Did you forget to push some commits? I'm still only seeing the 4 commits from a few days ago. |
tundal45
force-pushed
the
handle-nil-language-in-highlighter
branch
from
2f6e775
to
8f88526
Compare
|
@trek I had accidentally amended the travis ci commit with the spec changes. Fixed now. |
| else | ||
| return "" | ||
| end | ||
| return "" unless current_chapter |
There was a problem hiding this comment.
We could definitely remove the returns in the original, but I don't think this adds readability. Stylistically, I think return unless is fine for short circuiting a method, less OK for a doing so with return value, and not as communicative as if statements for multiple returns values.
There was a problem hiding this comment.
Makes sense. I'll switch back to the old version sans explicit returns.
tundal45
force-pushed
the
handle-nil-language-in-highlighter
branch
from
8f88526
to
01c2e11
Compare
|
@trek I need to squash the intermediary commits but this should have tests that use actual data rather than relying heavily on mocks & stubs. It also updates the highlighter to raise an error when there are nil languages rather than silently approving them. |
| @@ -11,6 +12,11 @@ def registered(app) | |||
|
|
|||
| module Helpers | |||
| def _highlight(string, language, class_name=nil) | |||
| error_message = "Code blocks must be fenced with proper language designated. If you don't know what language to use, specify text.\n\n" | |||
There was a problem hiding this comment.
"If you don't know what language to use, specify text" -> If you don't know what language to use, specify ```text
Inform the user that all code blocks must be fenced with proper language designation.
* Refactor few TOC methods after adding tests. * Introduce spec_helper
tundal45
force-pushed
the
handle-nil-language-in-highlighter
branch
2 times, most recently
from
cbf07c8
to
4362808
Compare
* Run the specs & trigger middleman build to catch build errors.
tundal45
force-pushed
the
handle-nil-language-in-highlighter
branch
from
4362808
to
040ccbc
Compare
|
@trek This addresses all the comments made here as well as those we discussed offline. I have travis running builds on my fork as well so that should work on the main repo once this gets merged in & setup on travis. The build is failing because some guides have to be updated to include language but I am going to open a separate PR to fix that once this is merged. I squashed the commits isolated units of work. |
|
@tundal45 I think the ordering of PRs is wrong. We'd need to wait for a the PR that adds languages to all language-less code blocks. If we merged this first, nobody could build the site! |
|
Otherwise, looks good! Thanks for all the hard work on this. I know it's been a lot of back and forth. |
|
@trek Happy to help. I will open the PR that fixes the guides with language-less code blocks shortly. |
Handle nil language in highlighter
|
yayyayyay! <3 |
I noticed that
middleman buildwas breaking so I thought it would be good to have that be executed as a part of the CI build. So I have addedtravis.yml. I believe there is an additional step in repository settings that needs to happen for this to work. I have also replaced the existing specs fortocwith unit tests that do not require a local middleman server running. The hope is that this will help with #10.