Update Mount Roulette to 3.1.0 #2090
Conversation
Add blacklist function.
| commands.blacklist = function(args) | ||
| local operation = args:remove(1) | ||
|
|
||
| if not operation then |
There was a problem hiding this comment.
Am I reading this right if you don't supply any arguments and just do //mr or //mr mount it would just list the blacklisted mounts?
There was a problem hiding this comment.
If you do //mr, line 162 defaults the command to mount. If you define the command, like //mr mount or //mr blacklist, the correct command is referenced from the commands table.
There was a problem hiding this comment.
Yeah, but removing if you don't provide a further option won't operation be nil here?
There was a problem hiding this comment.
Oh yeah, if you do //mr blacklist it just prints all blacklisted mounts, then returns.
There was a problem hiding this comment.
Oh nevermind me, I just realized what's happening here. I was misreading how the command handler works... sorry about that.
| @@ -43,6 +47,13 @@ for _, mount in pairs(resources.mounts) do | |||
| possible_mounts:append(mount.name:lower()) | |||
| end | |||
|
|
|||
| blacklist = {} | |||
There was a problem hiding this comment.
This should really be a set S{}. And so should settings.blacklist. In fact, not sure why you don't make settings.blacklist a set and use that directly. Does that not work for you?
There was a problem hiding this comment.
Ahh yes you're right! I think I had some issues with lists and saving them in settings (I think it was saving them as <0>...</0><1>...</1>), but since then I swapped to using sets, so I've changed this to just use the set directly as suggested and it works fine. Thanks!
| @@ -55,7 +66,10 @@ function update_allowed_mounts() | |||
| end) | |||
| local mount = possible_mounts[mount_index] | |||
|
|
|||
| allowed_mounts_set:add(mount) | |||
| -- Add this to allowed mounts if it is not already there and it is not blacklisted | |||
| if not allowed_mounts:contains(mount) and not S(blacklist):contains(mount) then | |||
There was a problem hiding this comment.
The not allowed_mounts:contains(mount) check is unnecessary.
|
|
||
| if not operation then | ||
| windower.add_to_chat(8, 'Blacklisted mounts:') | ||
| for k, v in ipairs(blacklist) do |
There was a problem hiding this comment.
If you make blacklist a set, you can use this for more convenient iteration:
for mount in blacklist:it() doSame below, no need to ipairs(T(allowed_mounts)).
|
You may want to consider making the list flexible between black and whitelist. Especially so that people that only want a few mounts in the roulette don't have to go and add a new mount every time one comes out. Just a thought. |
Use convenient iterator methods. PR feedback.
Great point. I've seen a whitelist/blacklist pattern in another addon before - do you happen to know which? Maybe I could |
AutoJoin has that pattern, not sure if that's the one you were looking for though. It has a |
|
Yep that's it. I'll look into this for the next thing. I'll default to blacklist as the mode (my assumption is that blacklisting a certain mount or two will be more utilised than a whitelist) so I'm ok for this PR to proceed and the whitelist option to follow later. |
|
@z16 If you agree, I think this should be ok to merge. The whitelist feature will come later. |
Adds the blacklist function.