-
Notifications
You must be signed in to change notification settings - Fork 129
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
Merge changes from 1010 and 1016 for oc_api #1020
Merge changes from 1010 and 1016 for oc_api #1020
Conversation
sync with 8.3 features branch
@NikkiLacrima Can you explain what this is for? #1010 is already merged, #1016 should merge in place. What's the difference with this? Additionally, as I understand with #1010 we're replacing runaway with runaway_confirmed as the link message that's sent when on runaway because previously it was being used twice. We've got a fair few uses of runaway in various other scripts (capture, for example, to immediately clear lists). Should we be aiming to do a final PR on feature freeze for 8.3 that updates all of these to runaway_confrimed? |
If you check #1016 it cannot be merged into the 8.3_Features-branch due to "This branch has conflicts that must be resolved". That is from the recent merging of 1010, since we were both working on oc_api. So what I did was resolve those conflicts. As for runaway_confirmed, handling "runaway" to clear the auth lists should IMHO not be done, instead the code should always listen to and use the link messages for adding/removing owners. |
Ah I see where you're coming from. When you have more than one edit to the same file there are going to be conflicts that need to be resolved, which is done simply via the web editor. When you see that "This branch has conflicts that must be resolved" message, you'll also normally see a button that says "resolve conflicts". That'll take you to an editor that lists all the conflicts and shows exactly where to resolve them, it's just a few minutes job doing it and avoids the risk of missing something in a manual merge. The best practice here is to merge the smaller edit first, then resolve conflicts with the larger merge via the resolve conflicts function. We will always update owner/trusted/etc. lists via the link messages for adding and removing owners regardless of runaway, as scripts will need to know if someone's added. My guess is the logic for clearing on runaway was in case someone's doing something in a menu at the same time, to ensure there's no delay in updating permissions. Right now if someone uses the chat command runaway, they will get the runaway menu to confirm, and if they click "Yes" the owner lists get cleared. However the owner lists temporarily stored in for example the oc_outfits script will be cleared on receiving the runaway command, and won't be reinstated on clicking "No". |
Going through the merge conflicts: 1st conflict is a part of comments and irrelevant. In short both of us fixed the # prefix thing separately, and Nikki's patch is an older version of the comments than current 8.3 branch version which I fixed to leave credit to Nikki as she did the patch before I did. Keep original. 2nd conflict is a spare blank line that Nikki removed. Nikki's correct, it should be removed, but it's just a stray blank line. I'm baffled Github sees this as a conflict. :D 3rd and 4th conflicts consist of refactoring difference. PR version:
8.3 version:
The difference is pretty minimal. Nikki's version assumes that CMD will be blank when there's a safeword command issued and then only processes the safeword check in that situation, whereas the current version processes the safeword every time. While Nikki's approach is definitely better for efficiency, I think it runs into a problem if the safeword starts with the prefix. For example, if someone's prefix is 're' and they type the default safeword "RED", cmd will be set to 'd', that gets processed, and the safeword is ignored. @NikkiLacrima am I wrong? If you agree we might as well close this PR, all the important stuff has been fixed in the previous merge conflict anyway. In future we could consider some code to disallow a safeword that starts with the wearer's prefix so that a efficiency improvement along these lines could be implemented. |
ok, with conflicts removed the changes now amount to differences in white space. Looks to me like all this is now successfully merged in through other PRs. @NikkiLacrima if I missed something please reopen and say so, but I think we've got everything in now and this PR becomes redundant, so closing. |
No new changes, should do what 1010 and 1016 does, basic sanity checks in world is done. Replaces 1016.