-
-
Notifications
You must be signed in to change notification settings - Fork 34
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add tests and refactor code #246
Comments
Makes sense. I think before you start we should properly respect an in-repo prettier config; iirc that's not happening at present. |
Yeah, and I keep needing to disable the "formatOnSave" setting because it makes my PRs get crazy with changes 😅 |
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 😄 |
I can do it, but not immediately: started a new job recently and haven't had tons of dev time outside it. |
Okay well I ended up doing it now https://github.com/kevboh/longform/tree/prettier |
Going to PR it and get it into main, then sleep! |
🤣 "I can't do it immediately" You're awesome @kevboh |
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.
The text was updated successfully, but these errors were encountered: