macteo / Marklight

Markdown syntax highlighter for iOS
Other
638 stars 44 forks source link

make Marklight processing pluggable #10

Closed DivineDominion closed 7 years ago

DivineDominion commented 7 years ago

While the MarklightTextStorage subclass works nice, if you have another external NSTextStorage subclass already, you cannot combine the two. This PR attempts to minimize the work you need to perform in your NSTextStorage subclass through delegation to a MarklightTextProcessor service instead of through inheritance.

(Works a bit towards #8 already, putting the style configuration in the MarklightTextProcessor and out of the text storage, where a style object can be extracted from later.)

I am not 100% happy with the name of the types I came up with, especially MarklightStyleApplier as a protocol that takes the minimum from NSMutableAttributesString, intending to make writing test doubles easier.

codecov[bot] commented 7 years ago

Codecov Report

Merging #10 into develop will increase coverage by 0.82%. The diff coverage is 88.15%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop      #10      +/-   ##
===========================================
+ Coverage    87.61%   88.44%   +0.82%     
===========================================
  Files            1        2       +1     
  Lines          210      303      +93     
===========================================
+ Hits           184      268      +84     
- Misses          26       35       +9
Impacted Files Coverage Δ
MarklightTests/MarklightTextProcessorTests.swift 90.32% <88.15%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 04bf7b2...f571496. Read the comment docs.

DivineDominion commented 7 years ago

Found a problem I still have to fix:

DivineDominion commented 7 years ago

@macteo fixed the problem. What do you think so far?

macteo commented 7 years ago

Seems good to me. I need to look at the code, but this can be probably be included in a new major version as there will be breaking APIs.

DivineDominion commented 7 years ago

I figured that won't be too much of a problem because it's 0.x anyway, but a 0.3 version jump makes sense to me. Tell me if I should increase it in the podspec and project settings.

DivineDominion commented 7 years ago

@macteo I finished an implementation for #6 based on this that's good to go 👍