Skip to content
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

Fix death message false positives #71

Merged
merged 3 commits into from
Dec 31, 2021

Conversation

MageLuingil
Copy link
Contributor

This fixes issue #70 by replacing the death keyword config option with a death message regex config option.

@destruc7i0n
Copy link
Owner

Nice work! Thanks for the report and fix.

Seeing as this is removing an option for the config, do you think you could add a message somewhere (maybe loadConfig in Shulker.ts) alerting those who may have the old config line that they need to update their config to correctly support this? Not sure myself about how many actively upgrade the bot, but would help those who do.

@MageLuingil
Copy link
Contributor Author

That sounds like a really good idea. I presume you're thinking just a warning to the console? I haven't tested with that option still present (and I'm still new to typescript) so I don't know if unexpected config options will throw errors.

In any case, I'll look into adding that when I have some time later.

@destruc7i0n
Copy link
Owner

Yeah just a warning for now. I should add some better config handling, to automatically add defaults for changes like these...

@MageLuingil
Copy link
Contributor Author

Okay, I added a configurable array for config option deprecation notices. Not sure if my way is the "best" way to do it in typescript, but there's at least a deprecation notice now, pointing people to the README.

Further feedback is of course welcome!

@MageLuingil
Copy link
Contributor Author

On a side note, my death message regex is based off of information from the Minecraft wiki (which has on occasion been wrong). Comparing my list to yours, it looks like I'm missing the keywords "finished", "speared", and "removed". I see "finished" in the list of removed death messages on the wiki (it appears it should have been handled by other keywords anyway), but I don't see any references to the other two.

Do you happen to know what the death messages for "speared" and "removed" were, and if I need to add them to my regex?

@destruc7i0n
Copy link
Owner

The best place to reference the death messages would be the language files. I attached a trimmed version of the latest en_us one for reference.

I do not see them either, not sure exactly where they came from.

@TheZackCodec had made the original PR for that, do you happen to know what those are form?

en_us.txt

@MageLuingil
Copy link
Contributor Author

MageLuingil commented May 19, 2021

Thanks for pointing out the language files to me! I don't know why I didn't think to use those from the start.

So, I may have slightly overkilled this, but I compiled a list of all death messages from the EN language files for all versions 1.12 - 1.16.5. The only message I was missing previously was an old typo in 1.12 for "[player] discovered floor was lava" (missing the "the").

I honestly don't know if this project works correctly with log files from 1.12, but I'll probably add support for that death message to my regex anyway. As for those other two messages, if they were ever in the game, it was probably only in snapshots.

Edit: I can go back farther than 1.12 btw, just wasn't sure when the log file messages were changed last.

@destruc7i0n
Copy link
Owner

The project came out in 2016 when 1.8.9 had released. I think that 1.12 is a good lower bound, however.

@destruc7i0n
Copy link
Owner

@MageLuingil Hey! Just wanted to check if you had a chance to update your regex as you mentioned above. (GitHub removes from homepage after 14 days so just wanted to keep this fresh on the page)

@MageLuingil
Copy link
Contributor Author

Hey @destruc7i0n! Sorry for the prolonged radio silence - I've been pretty busy IRL for the last two weeks. I do have that regex update, I just never got around to pushing it to the repo, sorry about that.

I was toying with the idea of combing through the death messages from 1.8 (or even 1.7, since that's one of the major modded versions) on up, but that was before life got busy.... Once I get some breathing room, I'll push the current fix, and then we might consider those additional messages later if you're interested (it really isn't that hard with a diff tool, just need to take some time for it).

@MageLuingil
Copy link
Contributor Author

Sorry for the delay; I added that missed death message.

I'm going to work on the death messages from 1.7-1.12 as well, but it may take a little longer.

@destruc7i0n destruc7i0n merged commit e489d05 into destruc7i0n:master Dec 31, 2021
@destruc7i0n
Copy link
Owner

destruc7i0n commented Dec 31, 2021

I've gone ahead and merged this, had another report which brought this to my attention again. Thanks again for the contribution! If you wish to update the death messages at another time, feel free to make another PR

@MageLuingil MageLuingil deleted the death-message-regex branch January 9, 2022 22:59
@MageLuingil
Copy link
Contributor Author

MageLuingil commented Jan 23, 2022

I made a script that compiles a list of all death messages across all release versions of the game, and checked that list against the regex currently being used. As of 1.18.1, we're catching all death messages (except for an unused one from 1.5 that later got removed). That means no more death message updates coming from me for now!

It also means future changes should be easier to find using this script (which I have here if you're interested).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants