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

Effects should be dumb, without any logic #3228

Merged
merged 1 commit into from
Feb 14, 2018
Merged

Conversation

janherich
Copy link
Contributor

@janherich janherich commented Feb 6, 2018

Works towards status-im/swarms#74 idea

Summary:

Coeffects/effects should be simple and not contain any logic other then implementation details about how the side effects are actually translated (whether it's direct call, queueing for later processing, batching, etc.) so they can be re-used to maximal extent.

Having actual application logic is them is bad because:

  1. They are less re-usable
  2. Hard to test

Also, I removed :message-overhead field in chat realm/app-db entity and every logic associated with it. It looks like some relict from older code, we were setting it, but it was never actually red.

As another thing, it fixes #3249

Testing notes (optional):

Chat regression testing

status: ready

@@ -173,4 +172,4 @@
:clear-history
(fn [{{:keys [current-chat-id] :as db} :db} _]
{:db (assoc-in db [:chats current-chat-id :messages] {})
::chat-events/delete-messages current-chat-id}))
:delete-messages current-chat-id}))
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason why you changed this to an unqualified keyword?

Copy link
Contributor Author

@janherich janherich Feb 6, 2018

Choose a reason for hiding this comment

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

Because implicitly qualified keywords are exceptionally bad idea when they are used outside the namespace where they were defined (and that was the case).
So it was ::delete-messages keyword defined in status-im.chat.handlers ns, and grep for for ::delete-messages revealed that the keyword is used only there - so far so good, but then I realised, then some other namespace status-im.ui.screens.group.chat-settings.events is actually importing the former namespace and refering to that keyword as ::chat-events/delete-messages....

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch! We should add this to guidelines somewhere, just to be aware if we see the same conundrum again

@status-github-bot status-github-bot bot moved this from REVIEW to TO TEST in Pipeline for QA Feb 6, 2018
@yenda
Copy link
Contributor

yenda commented Feb 6, 2018

I would move these events in the data-store with data-store.message namespace though

@janherich
Copy link
Contributor Author

janherich commented Feb 6, 2018

@yenda Correct, that's what I ultimately want to do once all data-store.message and data-store.chat side effect functions are called only from co-effects/effects and basically define the co-effects/effects there instead of those side-effect functions (that way, keeping both data-store.ENTITY and data-store.realm.ENTITY namespaces will finally have some purpose, public co-effects/effects in the former ns and db accessors in the later)

@status-github-bot status-github-bot bot moved this from TO TEST to CONTRIBUTOR in Pipeline for QA Feb 7, 2018
@janherich janherich force-pushed the feature/simple-effects branch 2 times, most recently from 32c7a43 to 34271f9 Compare February 7, 2018 16:54
@janherich janherich moved this from CONTRIBUTOR to REVIEW in Pipeline for QA Feb 7, 2018
@janherich janherich moved this from REVIEW to TO TEST in Pipeline for QA Feb 7, 2018
@status-im status-im deleted a comment from statustestbot Feb 9, 2018
@status-im status-im deleted a comment from statustestbot Feb 9, 2018
@status-im status-im deleted a comment from statustestbot Feb 9, 2018
@asemiankevich asemiankevich moved this from TO TEST to IN TESTING in Pipeline for QA Feb 14, 2018
@asemiankevich
Copy link
Contributor


Branch: PR-3228
Android: https://i.diawi.com/UdF6Cd
iOS: https://i.diawi.com/FwrGvR

@asemiankevich asemiankevich self-assigned this Feb 14, 2018
@asemiankevich asemiankevich moved this from IN TESTING to MERGE in Pipeline for QA Feb 14, 2018
@janherich janherich merged commit 86aea8a into develop Feb 14, 2018
Pipeline for QA automation moved this from MERGE to DONE Feb 14, 2018
@janherich janherich deleted the feature/simple-effects branch February 14, 2018 13:22
@asemiankevich asemiankevich moved this from DONE to Cherry pick for release in Pipeline for QA Feb 14, 2018
@yenda yenda moved this from Cherry pick for release to RELEASE 0.9.14 in Pipeline for QA Feb 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cleaned messages are shown if Clear History in group chat and relaunch app
7 participants