search

yogthos / markdown-clj

Markdown parser in Clojure
Eclipse Public License 1.0
540 stars 120 forks source link

Fix link and image issues #152

Closed dominicfreeston closed 5 years ago

dominicfreeston commented 5 years ago

There's an issue where if the link/image parser finds an opening [ but it's not a valid link of that type, it just bails out rather than ignore it, leaving the rest of the line unparsed. I added some tests that reproduced the issues to avoid future regressions.

It took me a little while to get my head around how make-link works and what was wrong with it, so I've also refactored it a little (flattened the if followed by cond into a single `cond) and left some comments.

~A slightly more contentious change was making all the helper methods private to the namespace, exposing only the transformers. Let me know if you'd rather I undid that in case other people might rely on them for some reason, but to me it makes the intention a bit clearer.~

I bumped the version number for convenience but can undo that as well if preferred. Let me know what you think, thanks!

yogthos commented 5 years ago

Hi, thanks for the PR. The change for make-link looks great. Regarding making things private, I generally prefer to keep functions public as they can be useful in other contexts. For example, it's possible to provide custom transformers, so you might use the helpers within those for example.

I typically find that it's better to use documentation and doc strings when clarification is necessary. So, I'd rather roll that change back, but otherwise I'm happy to merge the fix in and I can push out a new release.

dominicfreeston commented 5 years ago

Ok no problem, I dropped the offending commit 😄

yogthos commented 5 years ago

Fantastic, just pushed out 1.0.9 to Clojars. Thanks again for the PR.