Fixes #112 Fix game link orientation #388
Conversation
| // } | ||
| // ) | ||
| // | ||
| // // TODO: Add asserts on formatSpy - asynchronous nature of event handler breaks spies |
There was a problem hiding this comment.
Played around with this for a while - spies always get triggered before the async event handler gets called. Open to any ideas for potentially finding a way around it.
| .map(_.toLower) | ||
| .value() | ||
| const possibleSources = _.concat(sources, teamNames) | ||
| const teamNamesWithContext: SourceWithContext[] = sources |
There was a problem hiding this comment.
This was my first crack at a data model for passing around the source context - could potentially be extended to add whether notification came from a team (which could help solve #109). This is definitely the biggest change here.
| @@ -541,7 +597,7 @@ export async function tellMeWhenHandler( | |||
| // ----------------------------------------------------------------------------- | |||
| export async function helpHandler(bot: SlackBot, message: CommandMessage) { | |||
There was a problem hiding this comment.
Should we also have an explicit return type here that will help us catch if we don't return properly in the future? Not necessary for this PR, but in general, I'd like to see us rely less on the type-inference in cases like this.
There was a problem hiding this comment.
I'm all in favor of more explicit typing
There was a problem hiding this comment.
I don't mind type inference in callbacks, where it's obvious, but in top-level functions and the like, I prefer explicit typing.
There was a problem hiding this comment.
I think I'll open a ticket to go through all these handlers later, we should figure out what we want to do with them. Right now these callbacks are (bot, message) => void so we should probably decide if that's the paradigm we want to have in mind with them or if we want to clean them all up and start bubbling up promises for error handling and such
src/commands/subscription.ts
Outdated
| @@ -18,6 +18,17 @@ export const emitter = new ChessLeagueEmitter() | |||
| type Context = any | |||
There was a problem hiding this comment.
We should get rid of these. :( Fully meant to do that back when I first wrong them. we can turn on eslint no-explicit-any and fix from there.
There was a problem hiding this comment.
In this case, I think you should probably go ahead and get rid of this any for this change, to make sure we're probably using the context throughout.
| @@ -131,52 +168,73 @@ function formatNoTeamResponse() { | |||
| return "You can't use my-team or my-team-channel since you don't have a team right now." | |||
| } | |||
|
|
|||
| export function emitEvent(eventName: string, _league: League, sources: [string, string], context: Context): void { | |||
There was a problem hiding this comment.
It might even just make sense to get rid of the emitter pattern entirely, only thing it’s really using now is the reference to the bot object
No description provided.