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

Fishchimp/feature/delete unavailability #205

Merged
merged 3 commits into from
Apr 13, 2024

Conversation

fishchimp
Copy link
Contributor

Describe your changes

Slight improvement to the delete unavailability method

considerations:

  1. The remove_event method is already a soft delete, which aligns with industry practices
  2. The delete method is already quite straightforward
  3. Futureproofing ideas include changing the return type of remove_event into an enum to differentiate between a failed "mark event as deleted" and a "cannot find such event to delete".
  4. ultimately decided to just add logging for better debugging if any issues were to arise

Issue ticket number and link

Issue FE-187

emilymclean
emilymclean previously approved these changes Apr 2, 2024
@emilymclean
Copy link
Collaborator

emilymclean commented Apr 2, 2024

Just to pipe in a suggestion here, but if you're looking to pass failed event deletion up the chain I think throwing an exception would be entirely appropriate. I would subclass exception with "FireAppException", then subclass that with "ClientException" (like a caught validation exception, rather than a genuine bug in our implementation), and then subclass with as many exceptions as you need to represent every failed state. The reason for all the subclasses is so that the consuming endpoint can effectively adjust the scope of what it wants to catch.

Again, this is just a suggestion, feel free to implement how you feel fit.

naychithanshwe
naychithanshwe previously approved these changes Apr 3, 2024
@TaiHaDev TaiHaDev dismissed stale reviews from naychithanshwe and emilymclean via 47447bf April 13, 2024 01:49
@TaiHaDev
Copy link
Contributor

Hi @naychithanshwe @BenMMcLean , there was a minor conflict issue between Hafizh's branch and the main branch, so I helped him quickly resolve it. Please approve it so that it can be merged. Thanks!

@emilymclean emilymclean added this pull request to the merge queue Apr 13, 2024
Merged via the queue into main with commit 19e3373 Apr 13, 2024
6 checks passed
@emilymclean emilymclean deleted the fishchimp/feature/delete-unavailability branch April 13, 2024 11:04
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.

5 participants