Add front-matter writing capabilities

waiting-for-dev / front_matter_parser

Ruby library to parse files or strings with a front matter. It has automatic syntax detection.

MIT License

105 stars 12 forks source link

Add front-matter writing capabilities #8

Closed alsemyonov closed 5 years ago

alsemyonov commented 5 years ago

I have added a set of classes that provide abilities to write corresponding front matters to files. Also, I have introduced FrontMatterWriter and FrontMatter namespaces, since now it not only parses but writes also, so I believe event the gem could be renamed after all.

waiting-for-dev commented 5 years ago

Thanks for your contribution, it would be a great addition for this gem.

However, for better or worse, the gem's name is what it is, and I would not like to change it because of backward compatibility issues. Would you mind to refactor the PR in a way that current module naming is still used? For example, we could have two main classes FrontMatterParser::Parser and FrontMatterParser::Writer. We can discuss here the exact hierarchy beforehand, if you like.

alsemyonov commented 5 years ago

I haven’t had changed your namespace, if you look at changes. I just provided two namespaces for other possible names. Anyone could just use any of them: your old one, universal one or specific new one :)

Telegraphed from my Fridge

10 нояб. 2018 г., в 10:45, Marc Busqué notifications@github.com написал(а):

Thanks for your contribution, it would be a great addition for this gem.

However, for better or worse, the gem's name is what it is, and I would not like to change it because of backward compatibility issues. Would you mind to refactor the PR in a way that current module naming is still used? For example, we could have two main classes FrontMatterParser::Parser and FrontMatterParser::Dumper. We can discuss here the exact hierarchy beforehand, if you like.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

waiting-for-dev commented 5 years ago

I see. As you say, it is backward compatible because requiring FrontMatterParser acts just like before. However, I think the current implementation, even if very clever, is not a good idea for several reasons:

It doesn't follow the convention of gem name with top module name. In fact, now there are three top module names, so we are polluting namespace.
It is complex to understand and it doesn't follow the principle of least surprise (depending on what you require you get different capabilities for the FrontMatterParser module, which is weird).
It reflects the implementation history. Those new modules exist because at first there was only FrontMatterParser module.

I think it would be much simpler just to leave the FrontMatterParser top namespace.

waiting-for-dev commented 5 years ago

Great, thanks @alsemyonov . Let me some days to review the code and, if it is needed, start a code review to discuss with you the implementation details.

alsemyonov commented 5 years ago

Ok, sure!

I have rewritten commit history without introducing unneeded namespaces.

waiting-for-dev commented 5 years ago

Closing because the lack of feedback. But we can reopen it at any moment if you are still interested in this PR.