-
Notifications
You must be signed in to change notification settings - Fork 0
feat: configuration message #25
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.
Looks pretty good! Most of my comments are just related to code style/organization.
type OmitUnion<T, K extends keyof T> = T extends object ? Omit<T, K> : never; | ||
|
||
/** Create a configuration manifest for {@link DatabaseStore}. */ | ||
export const createConfigurationManifest = < |
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.
Is this function necessary? It appears that everything could just be rolled into ConfigurationMessage
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.
From what I can remember this is a function for type-safety reasons. I guess it's possible to do without it as well but that would probably result in a lot more verbose code.
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.
Why not put the same functionality in the constructor and write each object within the arguments of the constructor?
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.
That's possible but then you lose the flexibility of being able to use different database tables in a type-safe way.
src/repository/guild.repository.ts
Outdated
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.
Why this folder structure?
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.
This seems like a common file structure for database repositories to me. 🤔
What structure would you propose we use instead?
type PlaceholderReplaceHook = (context: PlaceholderReplaceHookContext) => string | null; | ||
|
||
/** Iterate over all placeholder values and replace with the result of the hook (or ignore if `null`). */ | ||
const placeholderIterateExecute = (text: string, hook: PlaceholderReplaceHook) => { |
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.
This should be possible to simplify quite a bit by using regex.
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.
The reason I did this with a lexer is so it can match [name]
in a string, but ignore [[name]]
(escaped), this is so it can be replaced with the literal [name]
text.
I can't immediately figure out a way to do this with regex, perhaps we could use simpler dollar sign placeholders? (e.g: $name
, $$name
to escape)
d42e861
to
8af27e9
Compare
This pull request adds a config command to configure the bot.
Closes #24