Store channels in array format #417
Conversation
xPaw
added
the
Type: Feature
xPaw
force-pushed
the
xpaw/channels
branch
2 times, most recently
from
82f0f61
to
a95eb89
Compare
| if (args.channels) { | ||
| channels = args.channels.map(function(chan) { | ||
| if (!chan.name) { | ||
| return; |
There was a problem hiding this comment.
We probably should print an error message there to let the user know that a channel has been ignored instead of just aborting like that.
There was a problem hiding this comment.
What would we even print though? "A channel with no name was ignored"? That doesn't seem particularly useful.
There was a problem hiding this comment.
Something like Channel X for network X for user Y is invalid, ignoring should do. The message doesn't have to be all that useful, it's just a matter of letting the user that something is wrong with their config, so the user isn't there "well, it loads my config fine and all but it doesn't join my channel!". I would be ok even with Invalid channel in config, the point really is just to give a hint to the user that an error happened.
Personally I would flat out crash (correctness > working), but that won't make a lot of happy people :P
|
Nice addition overall, consider this a 👍 as soon as my comment have been addressed (whether we decide to add a warning or not). |
|
@maxpoulin64 added a message and fixed a bug along with it. |
| "name" | ||
| ).join(","); | ||
| network.channels = _ | ||
| .filter(this.channels, {type: Chan.Type.CHANNEL}) |
There was a problem hiding this comment.
I know it's a copy from the above, but is it possible to use:
this.channels.filter(function(channel) { return channel.type === Chan.Type.CHANNEL; });? If so, let's do it? Please?
| _.filter(this.channels, {type: Chan.Type.CHANNEL}), | ||
| "name" | ||
| ).join(","); | ||
| network.channels = _ |
| log.warn("User '" + client.name + "' on network '" + args.name + "' has an invalid channel which has been ignored"); | ||
| } | ||
| // `join` is kept for backwards compatibility when updating from versions <2.0 | ||
| // also used by the "connect" window |
There was a problem hiding this comment.
I don't think we are converting channels on the fly until a new channel is added or removed (and re-ordered, maybe?) so that the list gets dumped in the user configuration. Or are we and I'm missing it?
Depending on the answer, can we log something like [WARNING] <user> is using the deprecated comma-separated channel list.? I realize you use the same code for handling old format and the join window input though, so not trivial if it stays that way.
There was a problem hiding this comment.
It gets converted when the user is loaded. And that warning isn't for comma list, it's for incorrect channel object. Comma list is silently converted below.
There was a problem hiding this comment.
Bottom-line is: is there a point soon where code L163 to L171 will be any obsolete?
There was a problem hiding this comment.
Not yet, it's used by the server window.
|
[Redacted, wrong PR...] |
|
@dgw: no, that one just prevents joining the server window. |
|
@maxpoulin64 I think he meant |
|
@xPaw Right, I did mean the 403 numeric. |
|
Sorry for the noise, commented on the wrong PR... 👍 and merging this, discussion in comment can continue post-merge. |
Store channels in array format
Preliminary support for #412.
