-
Notifications
You must be signed in to change notification settings - Fork 151
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
Allow templating realname #1374
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's an issue with the template validation.
config.sample.yaml
Outdated
# The format of the realname defined for users, either mxid or reverse-mxid | ||
# The format of the realname defined for users. | ||
# either "mxid", "reverse-mxid", or "template:TEMPLATE", where TEMPLATE must | ||
# contain either of $USERID, $LOCALPART, $DISPLAY, $IRCUSER |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is template limited to strictly "$USERID", "$LOCALPART", "$DISPLAY", "$IRCUSER" or can it contain prefix/suffix characters, as the help text isn't clear?
I think on the whole I would make mxid
and reverse-mxid
available as templating options inline with what you have there already (mxid is essentially $USERID) and then drop the whole template:
thing entirely, so the setting looks very similar to nickTemplate
in the end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can contain prefixes/suffixes (see the test for example).
Agreed, though; if I did this from scratch that's exactly how I would do it as well.
The reason for doing it this way is to keep backwards compatibility so users who have already configured this will keep having it working as previously. Though naturally this is at the expense of added complexity.
What do you think - keep as is or introduce a breaking change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We tend to favour dropping support for legacy options in the config, and adding backwards compatibility (with noisy warnings in the code instead).
matrix-appservice-irc/src/irc/IrcServer.ts
Lines 784 to 790 in 401c1ac
if (stringMappings.length) { | |
log.warn("** The IrcServer.mappings config schema has changed, allowing legacy format for now. **"); | |
log.warn("See https://github.com/matrix-org/matrix-appservice-irc/blob/master/CHANGELOG.md for details"); | |
for (const [channelId, roomIds] of stringMappings) { | |
config.mappings[channelId] = { roomIds: roomIds } | |
} | |
} |
At a later point, we can remove the old setting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds reasonable, I'll take a stab at that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How did the stabbing go?
73387cd
to
591fca0
Compare
@Half-Shot Sorry for leaving this hanging. I have updated it in line with your feedback, hopefully we can have better engagement this round ;) |
591fca0
to
cbad30c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me :) Thanks!
@jaller94 concern addressed? |
Hi, is there anything stopping the inclusion of this PR? The only last open review has been dealt with by adding a warning about using a legacy version of the configuration options, as far as I see, so everything seems good to go. |
Addresses #1095