Skip to content
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

feat(everything): Refactoring the way events are read to enable multiple event types #64

Merged
merged 5 commits into from
May 3, 2024

Conversation

baldm
Copy link
Contributor

@baldm baldm commented Apr 30, 2024

Why

We want to support more types of events from GitHub to send notifications. Now we should be able to easily implement new events by adding another file serializer.

What Has Changed:

  1. GithubPushContext has been decoupled from the Slack part of the program. This abstraction enables us to disconnect the file readers from the rest of the program and substitute them with alternative file readers. These can be found in the models/serializers directory.
  2. With the file serializers, we only need three different variables to populate the notification from the JSON: displayName, sha, and message. Therefore, a BaseGithubContext has been created to parse these values further into the application.
  3. The Slack notification, however, requires several more variables derived from the GitHub environment variables. This is where the GithubEvent class comes in to substitute the file reader GithubPushContext and simplify the passing of context.
  4. The _toMessage() method used for generating the Slack message previously in GithubPushContext has been moved to the SlackClient to decouple Slack-based functionality from the data classes and increase cohesion.
  5. FileUtils has been expanded and is now used for parsing different types of JSON events. The idea behind this change is to allow for reading various types of events while still permitting unknown JSON events without throwing an error. This solution will also allow for other GitHub events that might not be exactly the same but have the same fields to fit into the model. I initially wanted to accomplish this through inheritance, but I couldn't make it work. If you have better ideas, I'm open to feedback.
  6. Some methods have been renamed to increase readability, e.g., _generateMessage() to _generateSlackMessageFromEvent().

@baldm baldm marked this pull request as ready for review April 30, 2024 08:09
@baldm baldm requested a review from BrianEstrada April 30, 2024 08:09
@baldm baldm merged commit c575959 into main May 3, 2024
6 checks passed
@baldm baldm deleted the feat/rework-notification-building branch May 3, 2024 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants