Core: Only filter events from item/location name to id with id None

[duckboycool]

What is this fixing or adding?

Currently if you have a location/item id of 0, these will get automatically filtered out by the filter meant to remove events. This causes such IDs to not error in unittests since they are removed before running. I don't think there'd be a reason for any other falsy values besides None to be filtered either.

How was this tested?

Running the ID tests on a world that has an item or location with ID 0 to confirm it's now caught.

If this makes graphical changes, please attach screenshots.

0️⃣👎

[BadMagic100]

Time to start a fight over whether the existence of truthiness is a net benefit!

[duckboycool]

Perhaps another relevant PR is #4661, which moves to explicitly allow things besides None as "events". I think either way it's probably preferable to not let 0 be counted as an event though.

[ThePhar]

World locations or items with an id of 0 are technically against the AP spec anyway (purely to avoid this issue with AP or client-lib implementations), so world developers should not be doing this.

I thought it should also fail tests (for the world).

[Exempt-Medic]

I'll mention here as well that our current docs are inconsistent on whether or not we should even be filtering out None in the first place. Sometimes saying events should never go into these tables, and sometimes saying it's fine if they do. I'd personally rather see a change where even None errors out.

[qwint]

I'll mention here as well that our current docs are inconsistent on whether or not we should even be filtering out None in the first place. Sometimes saying events should never go into these tables, and sometimes saying it's fine if they do. I'd personally rather see a change where even None errors out.

i believe there was an attempted refactor to make None not get filtered and get explicitly checked for asserts and such, but it was objectively worse runtime and decided against, i thought it was a Treble PR but I didn't see it from a quick search, so it may have been fully tested in discord

I thought it should also fail tests (for the world).

the entire reason this pr was brought up was because someone was seeing unintended results and passing unittests, but I think the correct course of action is improve the test to find when people attempt to use invalid ids not change the runtime workflow
(maybe something along the lines of saving the provided dict to a different attr when testing to be checked raw)