MMRIZE / MMM-CalDAV

CalDav Parser
MIT License
8 stars 0 forks source link

Add backwards compatibility for calendars array in configuration #5

Closed alexiswatson closed 2 months ago

alexiswatson commented 10 months ago

First, thanks so much for putting this together! Exactly what I needed to stitch together a few Nextcloud calendars with the prototype I'm working on.

I saw the following in the change notes for the latest update:

  • calendars: [], is changed to targets: []. you have to reconfigure your config.js

I might infer this was done to support CardDAV, with a refactoring of configs to allow it to read in a more agnostic way (which makes total sense). Though someone looking at a minor release bump isn't likely to expect this kind of change. Rather, a breaking change would be the province of a major release (3.0.0) when using Semantic Versioning.

Rather than go through with a new major release, however, do you think it would make sense to simply alias calendars to targets for the 2.x branch, flagging that calendars is now deprecated in the docs? This would allow more natural-reading configs, while avoiding a breaking change.

If this is something you're open to, I'll see if I can't carve out some time for a PR over the next couple weeks.

eouia commented 10 months ago

You are right. I was debating 2.1, 3.0, and even 2.5. But my own rule for versioning my MM modules are; N. : need remove and re-install by whole restructuring. N.N : no need to re-install, but reconfiguration needed than previous version due to change of behaviors. N.N.N : just add additional new configuration besides legacy. No need to reconfiguration for legacy user. So I decided 2.1. It is not semantic versioning but consistent for me. Sorry for your inconvenience. :)

eouia commented 10 months ago

But if you want to make PR about compatibility with old legacy, always welcome.

alexiswatson commented 10 months ago

Appreciate the clarification. :> Adding backwards compatibility here shouldn't be too bad—out and about for a while, but I'll update the issue title to reflect the actual delta in the meantime.