-
Notifications
You must be signed in to change notification settings - Fork 66
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
Explicit category order #97
base: master
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.
Sorry I didn't see this sooner - I think it makes sense. There's just the extra complexity in category format caching that I don't entirely understand?
: null; | ||
|
||
formattedCategories.set(category, formattedCategory); | ||
|
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.
What are those changes for? 🤔
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 see what you mean; this cache is only being read in one place, so the computation of formattedCategory
should be moved down to line 458 where it is being used.
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 somewhat reverted the changes. I'm not entirely sure what formatMarkdownish
is doing. Which is better for commandsByCategories
keys: the original category, or the markdown-formatted category?
For both this and #99, do you have any guidance on writing tests? I skimmed the tests and I can see the |
Implements #96
WIP to gauge if this will be accepted before I add tests.