Skip to content

Conversation

@emorissettegregoire
Copy link
Contributor

@emorissettegregoire emorissettegregoire commented Dec 23, 2024

Allow message deletion by passing the message timestamp to the delete_message method

Copy link
Member

@matthewblack matthewblack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this point, we should be looking at all this code as the extendable version of Stealth. Even though, we don't have to build the actual functionality we shouldn't be making code that we will have to peal back later once we get to SMS/Bandwidth.

Hopefully that makes sense!

@emorissettegregoire
Copy link
Contributor Author

Yes absolutely, my bad. Will do! Thanks

@kladaFOX
Copy link
Contributor

DIdn't get the idea, so if I want to delete message I should call smth like this:
say(nil, thread_id: 'THREAD_ID', delete_message: 'MESSAGE_TS') right?

@emorissettegregoire
Copy link
Contributor Author

emorissettegregoire commented Dec 24, 2024

DIdn't get the idea, so if I want to delete message I should call smth like this:

say(nil, thread_id: 'THREAD_ID', delete_message: 'MESSAGE_TS') right?

In my use case I delete a mssg as soon as I send another one.. so I pass the mssg content as well. Check PR on Mav

@kladaFOX
Copy link
Contributor

yeah I get it you are calling
say("blablabla", thread_id: , delete_message:)
But why not implement delete_message(message_id)?
And your case would be more readable. You will call say and then delete or vice versa.
I also think that having the delete_message method would be better for future use cases

@matthewblack
Copy link
Member

matthewblack commented Dec 24, 2024 via email

@emorissettegregoire
Copy link
Contributor Author

Yes, thank you guys. I agree. Will check it with fresh eyes and see if we need both.

@matthewblack
Copy link
Member

Hey @emorissettegregoire - Sorry for the confusion earlier. I took a deeper look what you're doing with this delete_message argument via the stealth-slack client.

I was initially under the impression that this was one API call to slack and that's how they handled "replacing a message". However, it looks like you are sending two API requests - one for creating the message, and then another for deleting. This violates the Single Responsibility Principle (SRP) and I agree with Ilya that we should create a separate reply handler called delete and inside our Stealth app, we should call both.

The goal in Stealth is to make each function modular which makes it easier to maintain. Moreover, separating concerns makes it easier to test, debug, and extend functionality. For example, if we ever decide to introduce new deletion-related behaviors or parameters, we wouldn’t have to modify the say method and risk affecting unrelated logic.

Let me know if you want to chat through any implementation together! Happy Holidays!

@emorissettegregoire
Copy link
Contributor Author

Hey @emorissettegregoire - Sorry for the confusion earlier. I took a deeper look what you're doing with this delete_message argument via the stealth-slack client.

I was initially under the impression that this was one API call to slack and that's how they handled "replacing a message". However, it looks like you are sending two API requests - one for creating the message, and then another for deleting. This violates the Single Responsibility Principle (SRP) and I agree with Ilya that we should create a separate reply handler called delete and inside our Stealth app, we should call both.

The goal in Stealth is to make each function modular which makes it easier to maintain. Moreover, separating concerns makes it easier to test, debug, and extend functionality. For example, if we ever decide to introduce new deletion-related behaviors or parameters, we wouldn’t have to modify the say method and risk affecting unrelated logic.

Let me know if you want to chat through any implementation together! Happy Holidays!

@matthewblack on it. Thanks!

Copy link
Contributor

@kladaFOX kladaFOX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lovely!

Copy link
Member

@matthewblack matthewblack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hell yeah! Great work Emilie.

@emorissettegregoire emorissettegregoire merged commit 672dddf into 3.0-mountable Jan 14, 2025
@emorissettegregoire emorissettegregoire deleted the feature/delete-message branch January 14, 2025 17:19
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.

4 participants