Add optional display name suffix #433
Conversation
| @@ -87,7 +88,7 @@ export class SlackGhost { | |||
| }; | |||
| } | |||
|
|
|||
| public async update(message: {user_id?: string, user?: string}, room: BridgedRoom) { | |||
| public async update(message: {user_id?: string, user?: string}, room: BridgedRoom, config: IConfig) { | |||
There was a problem hiding this comment.
I recommend passing in the config in the constructor, and storing the suffix in a property rather than passing it into the update method.
There was a problem hiding this comment.
That was my original strategy but it became messy when I had to incorporate the tests. I could try doing it again though
There was a problem hiding this comment.
In my opinion it's a good thing to not have the config on the SlackGhost as long as only update uses it.
There was a problem hiding this comment.
Would still be nice to pass the suffix into the constructor then rather than the update method, imo. Alternatively if easier, pass the suffix into the update method. I'm not so keen on passing the whole config.
src/SlackGhost.ts
Outdated
| this.displayname = slackUser.profile.display_name; | ||
| changed = true; | ||
| if (slackUser.profile.display_name) { | ||
| const newDisplayname = slackUser.profile.display_name + config.display_name_suffix |
There was a problem hiding this comment.
Ideally please use template syntax
| const newDisplayname = slackUser.profile.display_name + config.display_name_suffix | |
| const newDisplayname = `${slackUser.profile.display_name}${config.display_name_suffix}` |
There was a problem hiding this comment.
@Half-Shot If we want to enforce this style, we should change the tslint.json.
Our current config:
"prefer-template": [
true,
"allow-single-concat"
],
It's auto-fixable.
https://eslint.org/docs/rules/prefer-template
There was a problem hiding this comment.
Yeah, let's do that in another PR!
This allows us to distinguish between Slack users and Matrix users. This is especially useful when using Matrix as a bridging hub. Signed-off-by: Dexter Chua <dec41@srcf.net>
|
On Wed, Jul 22, 2020 at 02:26:29AM -0700, Christian Paul wrote:
@dalcde The PR's description makes me wonder if you know of the config `username_prefix`.
It checks your Acceptance Criteria "This allows us to distinguish between Slack users and Matrix users.".
I am bridging matrix to other platforms. Since the local part that uses
the username_prefix is opaque, it is not suitable as a display name in
other platforms, so I must use the display name.
|
|
Yep, I mistook display name for username (commonly called "localpart" in Synapse). My bad. |
|
@dalcde Hiya, sorry this has been left to rot for two years. How would you feel about rebasing and finish off the changes? |
|
I am no longer using this, but other people are free to pick it up and finish it. Otherwise feel free to close the PR
|
This allows us to distinguish between Slack users and Matrix users. This
is especially useful when using Matrix as a bridging hub.