kevboh / longform

A plugin for Obsidian that helps you write and edit novels, screenplays, and other long projects.
Other
610 stars 30 forks source link

Add tests and refactor code #246

Open b-camphart opened 4 months ago

b-camphart commented 4 months ago

In approaching #244 I'm finding myself refactoring code to make it more testable to show that the new feature adheres to what was discussed there. I figured, if we're writing to people's files, we should be really, really sure we're doing it without messing anything up. However, all the refactoring felt more appropriate to do in a separate PR so that it's easier to review the incremental changes. With that in mind, I'm proposing to refactor the code, and add tests for, the following use cases:

My thinking is to have these each as their own PRs (which the subtasks can be linked to in github, if I'm not mistaken). However, I wanted to document my intention here because the way I'm planning to refactor the code would apply across each of these. My plan is to add these files to the src/model folder:

These files would contain the refactored code that handles the functionality for the above use cases, but would be completely isolated from the obsidian api, since it's basically impossible to do unit tests against without fully mocking out the dependency. In many cases, I'd simply be moving functions that were previously defined in other src/model files, then re-exporting them in the place they used to be so that references across the codebase don't need to be updated. In some cases, I might have to adjust the functions to accept an interface, which would serve as a wrapper around the obsidian api, but would allow easy mocking in unit tests.

I'll create a PR for the first use case shortly so that you can see more of what I mean, @kevboh . Just want to get your thoughts on this before I did a whole bunch of stuff. I figure, this issue could also serve as a discussion point for further refactorings and tests being added for other use cases in the future, if you want to go this route.

kevboh commented 4 months ago

Makes sense. I think before you start we should properly respect an in-repo prettier config; iirc that's not happening at present.

b-camphart commented 4 months ago

Yeah, and I keep needing to disable the "formatOnSave" setting because it makes my PRs get crazy with changes 😅

b-camphart commented 4 months ago

Would you want to do that? I can, if not. It's just that, it might require a lot of files being updated to match the rules, so the PR could be a nightmare. If you do it, no need for a PR 😄

kevboh commented 4 months ago

I can do it, but not immediately: started a new job recently and haven't had tons of dev time outside it.

kevboh commented 4 months ago

Okay well I ended up doing it now https://github.com/kevboh/longform/tree/prettier

kevboh commented 4 months ago

Going to PR it and get it into main, then sleep!

b-camphart commented 4 months ago

🤣 "I can't do it immediately" 20 minutes later "well, I did it"

You're awesome @kevboh