New room event #266
New room event #266
Conversation
Codecov Report
@@ Coverage Diff @@
## alpha #266 +/- ##
==========================================
- Coverage 53.05% 51.68% -1.37%
==========================================
Files 74 74
Lines 2754 2790 +36
Branches 408 421 +13
==========================================
- Hits 1461 1442 -19
- Misses 1293 1348 +55
Continue to review full report at Codecov.
|
* Move AppInterface definition * Lock events on apps load
| @@ -51,6 +51,13 @@ export class AppStatusUtilsDef { | |||
| return false; | |||
| } | |||
| } | |||
|
|
|||
| public isError(status: AppStatus): boolean { | |||
| return [ | |||
There was a problem hiding this comment.
This is a micro-optimization, but wouldn't it be better to have this array created static and as a constant instead of having to be created every time this method is called?
There was a problem hiding this comment.
Indeed it would be slightly more efficient, indeed it is a micro-optimization. IMO it's clearer to read it that way, everything is inside the method's body.
If time comes we need this list elsewhere, no doubt we should extract to a static array. Right now I'm more inclined to go with the "clearer" version
There was a problem hiding this comment.
Interesting, I’m of the opposite opinion. I think it is more clear to read the code when it is reading a named variable/constant that expresses what it is for than to declare it in the body of the method being called.
There was a problem hiding this comment.
Taking what you said as a rule is generally safer, but in this specific case, we have a bunch of things helping us: short method body; short method name; clear enum items named inside the array; few items inside it.
But should the situation be different, it would be worth it to go down a different path 😃
What? ⛵
IPreRoomUserJoinedeventIPostRoomUserJoinedeventAppsEngineErrorclass that signals prevention from apps eventsWhy? 🤔
The new event will allow apps to provide some access control to rooms.
Links 🌎
RocketChat/Rocket.Chat#17487
PS 👀