add an option to merge default configs

gjtorikian / commonmarker

Ruby wrapper for the comrak (CommonMark parser) Rust crate

MIT License

429 stars 82 forks source link

add an option to merge default configs #315

Open monkeyWzr opened 2 weeks ago

monkeyWzr commented 2 weeks ago

Fix #314

gjtorikian commented 2 weeks ago

Heya! This looks good, but I'm about to head on vacation and the only change I'd request is for merge_default_options to be removed. You're right in that it's more natural/correct to expect that the rest of one's defaults remain unchanged; so the default here should be to always merge in the defaults (and respect the user's config choice otherwise).

monkeyWzr commented 2 weeks ago

Enjoy your vacation!

the only change I'd request is for merge_default_options to be removed

Sure! I'm happy to make the changes but there are two things:

tons of tests need to be fixed
it might break user's app if someone is relying on the current behavior

For the first one I think I can find a way out. For the second I really don't know what I can do so I'm gonna leave it to you 😄 (maybe some breaking change notes in the changelog?)

gjtorikian commented 5 days ago

Hello! Just dropping a note to say I'm back from vacation.

I started to take a look at this and, yowza, you're completely right, the options merging is a bit of a mess. I've got a few dozen spec tests to fix, but I do think I will make option merging the default in a new major version. Thanks for pointing this out! I can't believe it's been like this for so long without anyone noticing. 😓