andrewbrereton / obsidian-to-ical-plugin

This is a plugin for Obsidian that searches your vault for tasks that contain dates, and generates a calendar in iCal format that can be imported into your preferred calendar application.
MIT License
61 stars 13 forks source link

(review) Architecture, roles and cleanliness #48

Open Blondwolf opened 8 months ago

Blondwolf commented 8 months ago

Based on this post.

The ticket is to discuss and treat architecture problems / design patterns / standards and eventual cleaniness problems.

Blondwolf commented 8 months ago

Two subjects here first. More can be added.

1) Thinking about entrypoint and Main

In Main, start() is just the main entrypoint. I know the Obsidian sample plugin treats Main similar to how I treat ObsidianIcalPlugin. However I wanted ObsidianIcalPlugin to only deal with the Obsidian integration, and Main to be the entrypoint to the business logic of the plugin.

The question here is to know if creating a new Main dedicated main for the plugin is an overkill. I think it is not really a problem for the moment and with the actual plugin scope. Maybe the problem could belong then to registering to specific events. If you want to add more logic as unload then you have to call a new function in your main like unstart or whatever. The problem is then you need to reference all functionnalities where this is already built in the extends Plugin.

Though, I can understand the will to separate the whole settings management from the plugin logic.

I am not sure either this is a good idea to change that right now. Also new to Obsidian plugins so don't the real standards. We will see by doing I think. We should keep it as is for the moment.

2) About some class roles

createTaskFromLine() is a factory method to generate Tasks from strings. I agree that passing settings around like that is not ideal.

So I think it make sens and not much changes to move this function to TaskFinder directly as it's more his role to extract the information than the Task class itself. Task class should keep to be only a model. The only thing you could add like that in the model is a copy constructor but it's YAGNI right now.


So all you should do right now it some moving this createTaskFromLine