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

Many global strings have an unnecessary space between the unit's name and the possessive " 's " #26

Open
balakethelock opened this issue Jan 6, 2024 · 2 comments

Comments

@balakethelock
Copy link
Contributor

A quick ctrl+f search in https://github.com/YamaYAML/LegacyPlayersV4/blob/45bcf125bf623404721d6420089c397ff0088d69/Addons/AdvancedVanillaCombatLog/core.lua#L567C45-L567C45
shows 84 instances of a space being placed between unit name and the " 's "

This breaks a lot of addons that need to read events from combat log. Example: Bigwigs

@melbaa
Copy link
Contributor

melbaa commented Jan 18, 2024

There's also an advantage of having the extra space, it makes the log file grammar a bit less ambiguous to parse, because the 's is never part of an interesting token and serves as a separator.
eg
Earthbind Totem 's Earthbind was evaded by Anvilrage Footman.
when you see the " 's " you know the npc/player name has ended.
while with
Earthbind Totem's Earthbind was evaded by Anvilrage Footman.
it's not obvious what Earthbind Totem's Earthbind is, because ' is allowed inside npc names. Harder to tell when the npc name ends and spell name starts. You need context or lookahead/lookbehind lookups or multiple passes over the line to parse it all and then to remove the optional trailing 's from names.

The legacyplayers code currently considers 's as part of a name to be an error.
https://github.com/YamaYAML/LegacyPlayersV4/blob/8cd83675602a1ba6df34e58a034f4539f1d7bd6e/Backend/src/modules/live_data_processor/tools/cbl_parser/wow_vanilla/parse_unit.rs#L20-L21

parsing would have to change a bit https://github.com/YamaYAML/LegacyPlayersV4/blob/8cd83675602a1ba6df34e58a034f4539f1d7bd6e/Backend/src/modules/live_data_processor/tools/cbl_parser/wow_vanilla/parser.rs#L97-L142

And then there's the question of backwards compatibility versus invalidating all old logs + telling everyone to update the combat log addon they have.

@YamaYAML
Copy link
Collaborator

Interesting find! As each upload is parsed during upload the damage is done but we should definately look into the logger for future use and less breaking of other add-ons. Tbc

@YamaYAML YamaYAML pinned this issue Jan 27, 2024
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

No branches or pull requests

3 participants