-
Notifications
You must be signed in to change notification settings - Fork 68
NotesFactory and Tasks function #655
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mlevison This is a great start. Thanks! A starting observation:
- For generalized use, the term "note" is misleading here, because these objects you created are not notes, they are objects with paragraphs arrays. In your tests, you are just accessing that one attribute, which makes sense. However the term "note" is an important distinction because if I wanted to construct a test, I would expect a note to have a title, type, filename, frontmatterAttributes, etc. This is why I was suggesting you use JEST helpers to give you a quick/easy way to create basic note attributes in JSON form.
- Also, defining them all in a single file may get unwieldy if we add lots and lots of them over time. you may want to search the code for the string "const factory = async" to see how factory files are loaded dynamically in Templating by Mike. this way, each test file is actually its own file and does not have to be concatenated in a never-ending single file. That said, I don't like that the Templating factory loader code is import code is repeated. should definitely be a shared function.
There are a always a lot of ways to deal with these two things. Look forward to hearing your thoughts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And on a different 'note' (ha ha), I would even at this early stage suggest adding a richer set of content. I.e. note links, dates, emojis, URLs, Markdown links, non-Roman characters. Each of these has caused issues in the past.
Happy to make improvements.
|
@mlevison I'm totally with you on refactoring once a file gets large, but please do have a look at the factories loading in the np.Templating plugin per my previous comment. I think it would be better architecturally to have each note JSON as its own file and then dynamically load them by name when needed rather than having all the notes as a const in one file. |
@mlevison I just realized that frontmatter attributes was not yet included in the output from JestHelpers so I am pushing that to main. please pull the latest from main and update your branch with this new version. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added one comment, about 'tricky' content, but otherwise I'm OK with it so far.
This is the start of the refactoring I proposed in discord.
I created a NotesFactory - since some of the examples are borrowed from helpers/tests/paragraph.test.js - it would easy to refactor.
I added tasks.js to helpers and I intend to migrate the other task related functions there as I find them. I.E. from sorting, getAllTasksByType()