Fix for issue #66

[LCWilliams]

Fix replaces the issubclass check with an isinstance check. This fixes the issue of #66

[leonhard-s]

I unfortunately cannot add suggestions to files that were not changed, but here are two tests that could be appended to auraxium/tests/event_test.py to cover both a regression for #66 and a to-do for fixing the auto-insertion of logicalAndCharactersWithWorlds when event names are used instead of types (see https://github.com/leonhard-s/auraxium/pull/67#discussion_r1204684620):

    def test_regression_66_string_event_with_world_filter(self) -> None:
        """https://github.com/leonhard-s/auraxium/issues/66"""
        trigger = auraxium.Trigger('ContinentLock', worlds=[1])
        _ = json.loads(trigger.generate_subscription())

    def test_logical_and_autoselect(self) -> None:
        """Test the auto-insertion of the logicalAnd flag."""

        # TODO: Support logicalAnd* auto-insertion for triggers defined using
        # event names rather than types

        event_variants = (auraxium.event.Death,)
        # Character-centric event with no filter -> no logicalAnd
        for event in event_variants:
            trigger = auraxium.Trigger(event)
            data = json.loads(trigger.generate_subscription())
            self.assertNotIn('logicalAndCharactersWithWorlds', data)
        # Character-centric event with world filter -> logicalAnd
        for event in event_variants:
            trigger = auraxium.Trigger(event, worlds=[1])
            data = json.loads(trigger.generate_subscription())
            self.assertSequenceEqual(
                data['logicalAndCharactersWithWorlds'], 'true')
        # Character-centric event with character filter -> no logicalAnd
        for event in event_variants:
            trigger = auraxium.Trigger(event, characters=[1])
            data = json.loads(trigger.generate_subscription())
            self.assertNotIn('logicalAndCharactersWithWorlds', data)

[LCWilliams]

Aha, of course it wouldn't be as simple as changing one line. :p

Shall attempt to look into this further! Still trying to understand how this library (and pull requests :x ) works. :grin:

[LCWilliams]

I've done some more fiddling around with this and I believe I've managed to achieve a solution that behaves with both type specified and string specified parameters; while also not needing to do any refactoring~

[leonhard-s]

Could you update event/event_test.py with the test cases from https://github.com/leonhard-s/auraxium/pull/67#issuecomment-1561845671 and add it to the PR?

[LCWilliams]

Requested changes have been made! :smile:

[leonhard-s]

Here are some suggested alterations to both fix the breaking unit tests and expand the test case to also handle the custom experience ID filters for GainExperience:

tests/event_types.py

    def test_logical_and_autoselect(self) -> None:
        """Test the auto-insertion of the logicalAnd flag."""
        event_variants = (auraxium.event.Death, "Death",
                          'GainExperience', 'GainExperience_experience_id_1')
        # Character-centric event with no filter -> no logicalAnd
        for event in event_variants:
            trigger = auraxium.Trigger(event)
            data = json.loads(trigger.generate_subscription())
            self.assertNotIn('logicalAndCharactersWithWorlds', data)
        # Character-centric event with world filter -> logicalAnd
        for event in event_variants:
            trigger = auraxium.Trigger(event, worlds=[1])
            data = json.loads(trigger.generate_subscription())
            self.assertSequenceEqual(
                data['logicalAndCharactersWithWorlds'], 'true')
        # Character-centric event with character filter -> no logicalAnd
        for event in event_variants:
            trigger = auraxium.Trigger(event, characters=[1])
            data = json.loads(trigger.generate_subscription())
            self.assertNotIn('logicalAndCharactersWithWorlds', data)

event/_trigger.py

        elif (self.worlds and not self.characters):
            char_events = [s.__name__ for s in CharacterEvent.__subclasses__()]
            event_names = [e if isinstance(e, str) else e.__name__
                           for e in self.events]
            if any((e.startswith('GainExperience_experience_id_')
                   or e in char_events) for e in event_names):
                json_data['logicalAndCharactersWithWorlds'] = 'true'
        return json.dumps(json_data)

[LCWilliams]

I just applied those changes and the local tests ran okay! Comitting them in a moment~

[leonhard-s]

I believe the "failing" test is just the code coverage upload, which is not supported from forks due to missing tokens (see #41, still haven't gotten around to fixing that). I'll see if I can fix this on the main repo, otherwise I'll just merge manually later. I think the PR looks good now 👍

[LCWilliams]

aha. :stuck_out_tongue_closed_eyes: always has to be 'one more thing' that keeps us chugging away at these things! and happy to help if I can! :stuck_out_tongue_winking_eye:

[leonhard-s]

I believe I have a fix for the workflow runs on forks. Could you merge the develop branch from this repository into your PR to confirm? (GitHub Docs: Updating a fork with upstream commits)

[leonhard-s]

Code coverage is still failing (it running as part of a PR still makes it part of this repo, which is why my fix did not work), but separating the two at least tells us that everything's working - I'll do any other testing for #41 separately, this is not related to this PR anymore.

Thank you for the PR! I will prepare a new PyPI release over the weekend. If you want to already use the current development version for your project, you can force pip to install straight from the branch with python -m pip install git+https://github.com/leonhard-s/auraxium@develop