-
Notifications
You must be signed in to change notification settings - Fork 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
Centralized file for messages #91
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.
You don't need to include : string
in every declaration, it is inferred
Also, instead of
const message = 'text';
export { message };
you can just do
export const message = 'text';
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.
We need a way to store messages with templates too, not sure how yet but this can be discussed.
We can just export functions instead of strings from the file // template
export const templateMessage = (a: string) => `${a} is used here`;
// static
export const staticMessage = () => 'static message'; |
This won't allow us to use the Discord.js toString() overrides as TypeScript will be mad. p.s. haven't slept for a while, gonna write in more detail tomorrow. |
In that case I think using a message class would be a perfect solution. export class Message {
functionWithStaticMessage(): string { return 'some text'; }
functionWithParameter(a: string): string { return 'value of a is' + a; }
} |
@robertt Thoughts? Personally I don't really like this since you have to make an instance of the class and the |
Consider: export const messages = {
staticMessage: () => 'static message here',
templateMessage: (a: string) => `this is a template, ${a}`
} Short lines and simplicity. |
@jellz 👍, would probably be worth putting it in an object, so for example: const messages = {
commands: {
rep: {
toSelf: () => ':x: you cannot send rep to yourself',
success: (member: string) => `:ok_hand: successfully sent rep to ${member}`
}
},
}; |
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.
LGTM other then the previous unresolved comment but we still need to figure out how to organize the actual messages nicely.
p.s. we should probably consider future i18n too
Anything blocking this from being merged? |
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.
In addition to the review comments I'd also like to mention you should try to make the constant's names a bit more detached from the sentence.
E.g. beAskerToCloseChannel
-> noChannelClosePerms
/ cannotSendRepToYou
-> noSelfRep
. (or something like that)
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.
Good enough.
Centralized file for messages #90