Removing direct access to Account.db._playable_characters in favor of… #3179

volundmush · 2023-05-06T06:33:25Z

Brief overview of PR changes/additions

Removing direct access to Account.db._playable_characters in favor of Account.add_character(char) and Account.remove_character(char). Account.characters already handles all filtering so am cleaning up lots of repeated list comprehensions which remove/filter deleted characters.

Motivation for adding to Evennia

There's a lot of old code which should probably be cleaned up, and managing which characters are bound to an account is logic that should not be duplicated all over the place in every command. now we have a simple method-based API to handle that task.

Other info (issues closed, discussion etc)

I am unsure if I should modify the tests to use the API. many of them directly access .db._playable_characters. I'm sure SOME could be changed...

Griatch

I think this makes for a nice cleanup, but concern is as usual with the naming ... the thing about _playable_characters is that it's a soft listing of characters you may play. Its main purpose is not to limit who you can play (that's down to the puppet: lock, but to allow us to show a nice list of characters as a reminder (in the character selection screen).
I fear the add_character etc naming may suggest that you are creating a stronger link between the Character and the Account than you are.

To make this explicit, maybe this may be better:

add_character_to_playable_list
at_post_add_character_to_playable_list
remove_character_from_playable_list
at_post_remove_character_from_playable_list

It's longer, but makes it much clearer what is actually happening. I'm open to other suggestions though.

But either way, if we do this change, the unit tests should change to make use of the new best practice as well.

Griatch

I think the changes look good overall, I think the renames were done everywhere (but it's a bit hard to track).

The unit tests doesn't pass for this.
The install and adding of the Rich library (EvRich) is way beyond the scope of this PR, please remove that. While we may want to add it at some point, it needs to be reviewed separeately.

As a side note, please avoid squashing your commits before review is complete. It throws away the history and means the reviewer has to basically do the entire review all over every time (After approval, one can squash for a better merge history, but we haven't worried too much about that to make it easier for contributors).

Griatch · 2023-05-21T08:02:58Z

evennia/__init__.py

@@ -363,6 +364,9 @@ class SystemCmds(_EvContainer):
    del SystemCmds
    del _EvContainer

+    # Trigger EvRich to monkey-patch Rich in-memory.
+    install_evrich()


This does not belong with the PR?

Oops I don't know how that snuck in there.

Griatch · 2023-06-04T07:15:58Z

@volundmush Ping.

volundmush · 2023-09-09T19:05:06Z

Removed the Rich stuff; not at all sure how it snuck in there, I probably got a few commits crossed when experimenting with other things.

Griatch

I like the changes to how to operate the db._playable_characters - using helper methods makes for less code. I have some further comments inline, but two main ones:

Looking at how these helper methods are used here, in combination with self.characters makes me wonder why .characters is not a handler. Since people are encouraged to look at self.characters now to see which playable characters they have, it's not at all obvious that you need to call add/remove_character_to/from_playable_list to modify .characters (if you don't know how it works under the hood). It sounds like it would be more intuitive to let .characters be a handler so you'd do self.characters.add()/.remove(). Drawback of that is that you'd need to do .characters.get() to get the characters (so a backwards-incompatible change), but it would be more in line with how these things work elsewhere.
There are a few additions here that seems out of scope of the PR still, including some more Rich-references.

Griatch · 2023-09-10T13:25:17Z

docs/source/Howtos/Turn-based-Combat-System.md

@@ -389,7 +389,7 @@ def resolve_combat(combat_handler, actiondict):
    for (char, fleevalue) in flee.items():
        if fleevalue == 2:
            combat_handler.msg_all(f"{char} withdraws from combat.")
-            combat_handler.remove_character(char)
+            combat_handler.remove_character_from_playable_list(char)


Why is this change needed on the combat_handler? That's a Script, not an account? Besides the combathandler's remove_character was not updated either. So this looks like an search-and-replace gone wrong.

Griatch · 2023-09-10T13:27:06Z

docs/source/Howtos/Turn-based-Combat-System.md

            return
        target = self.caller.search(self.args)
        if not target:
            return
        # set up combat
        if target.ndb.combat_handler:
            # target is already in combat - join it            
-            target.ndb.combat_handler.add_character(self.caller)
+            target.ndb.combat_handler.add_character_to_playable_list(self.caller)


Same here. combat_handler is a Script...

Griatch · 2023-09-10T13:27:44Z

docs/source/Howtos/Turn-based-Combat-System.md

            target.ndb.combat_handler.msg_all(f"{self.caller} joins combat!")
        else:
            # create a new combat handler
            chandler = create_script("combat_handler.CombatHandler")
-            chandler.add_character(self.caller)
-            chandler.add_character(target)
+            chandler.add_character_to_playable_list(self.caller)


Same here, it doesn't make sense to rename this, especially since the combathandler's code was not updated to match (and it's unrelated to this anyway)

Griatch · 2023-09-10T13:29:34Z

evennia/accounts/accounts.py

+    def add_character_to_playable_list(self, character: "DefaultCharacter"):
+        """
+        Add a character to this account's list of playable characters.
+        """


It's worth to expand the docstring here, since this list is not really limiting what can be played (the puppet: lock does). The list is only used as a quick way to show a list on the character-selection screen. Not to mention that this listing is not unique - any number of people can have a character in their playable lists.

Griatch · 2023-09-10T13:37:14Z

evennia/commands/default/unloggedin.py

@@ -465,3 +465,64 @@ def func(self):
                utils.get_evennia_version(),
            )
        )
+
+
+def _create_account(session, accountname, password, permissions, typeclass=None, email=None):


This entire modification seems out of place in this PR? Besides, this looks like a duplication of the Account.create code ...

Griatch · 2023-09-10T13:38:37Z

evennia/server/serversession.py

+            if hasattr(text, "__rich_console__"):
+                text = self.print(text)


This again doesn't belong here - Rich was stripped out...

Griatch · 2023-09-10T13:38:47Z

evennia/server/serversession.py

+        if (t := kwargs.get("text", None)):
+            if hasattr(t, "__rich_console__"):
+                kwargs["text"] = self.print(t)


Does not belong with this PR ...

Griatch · 2023-09-10T13:45:31Z

evennia/commands/default/unloggedin.py

+    return new_account
+
+
+def _create_character(session, new_account, typeclass, home, permissions):


This looks like it's doing what Account.create_character does with some more reporting ...

volundmush · 2023-09-11T15:06:21Z

Let's do this!

Griatch · 2023-09-20T20:51:23Z

@volundmush Is this ready to re-review?

Griatch · 2023-10-29T06:24:42Z

@volundmush This and other older PRs need rebasing since we did a cleanup of the repo lately (reducing its size). Sorry about that.

… Account.add_character(char) and Account.remove_character(char). Account.characters already handles all filtering so am cleaning up lots of repeated list comprehensions which remove/filter deleted characters.

…aracters.

…e characters to use it.

volundmush · 2023-10-29T17:29:25Z

@volundmush This and other older PRs need rebasing since we did a cleanup of the repo lately (reducing its size). Sorry about that.

Rebase applied.

Griatch

Thanks for the cleanup. I think this looks fine now.

Griatch requested changes May 7, 2023

View reviewed changes

volundmush force-pushed the player_character_management branch from d2060f2 to 7535620 Compare May 8, 2023 01:28

Griatch requested changes May 21, 2023

View reviewed changes

volundmush force-pushed the player_character_management branch 3 times, most recently from 1d94f70 to 01c1e67 Compare September 9, 2023 18:39

Griatch requested changes Sep 10, 2023

View reviewed changes

Griatch force-pushed the main branch from 72554c5 to dd38f7d Compare October 19, 2023 19:23

volundmush added 6 commits October 29, 2023 13:02

Removing direct access to Account.db._playable_characters in favor of…

f782cd8

… Account.add_character(char) and Account.remove_character(char). Account.characters already handles all filtering so am cleaning up lots of repeated list comprehensions which remove/filter deleted characters.

Cleaned up tests to use newly-renamed Account hooks for add/remove ch…

4b80b20

…aracters.

Fixed a borked test.

376d1d1

Cleaning up GlobalScriptContainer from some junk logic.

c918226

Removing more erroneous code that snuck in.

3c4a3f1

Added CharactersHandler to account and altered all calls of add/remov…

2bf96f7

…e characters to use it.

volundmush force-pushed the player_character_management branch from 338d751 to 2bf96f7 Compare October 29, 2023 17:03

volundmush requested a review from Griatch October 29, 2023 17:29

Griatch approved these changes Oct 31, 2023

View reviewed changes

Griatch merged commit 04e0852 into evennia:main Oct 31, 2023
7 of 8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removing direct access to Account.db._playable_characters in favor of… #3179

Removing direct access to Account.db._playable_characters in favor of… #3179

volundmush commented May 6, 2023

Griatch left a comment •

edited

Griatch left a comment

Griatch May 21, 2023

volundmush May 21, 2023

Griatch commented Jun 4, 2023

volundmush commented Sep 9, 2023

Griatch left a comment

Griatch Sep 10, 2023

Griatch Sep 10, 2023

Griatch Sep 10, 2023

Griatch Sep 10, 2023

Griatch Sep 10, 2023

Griatch Sep 10, 2023

Griatch Sep 10, 2023

Griatch Sep 10, 2023

volundmush commented Sep 11, 2023

Griatch commented Sep 20, 2023

Griatch commented Oct 29, 2023

volundmush commented Oct 29, 2023

Griatch left a comment

		if hasattr(text, "__rich_console__"):
		text = self.print(text)

		return new_account


		def _create_character(session, new_account, typeclass, home, permissions):

Removing direct access to Account.db._playable_characters in favor of… #3179

Removing direct access to Account.db._playable_characters in favor of… #3179

Conversation

volundmush commented May 6, 2023

Brief overview of PR changes/additions

Motivation for adding to Evennia

Other info (issues closed, discussion etc)

Griatch left a comment • edited

Choose a reason for hiding this comment

Griatch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Griatch commented Jun 4, 2023

volundmush commented Sep 9, 2023

Griatch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

volundmush commented Sep 11, 2023

Griatch commented Sep 20, 2023

Griatch commented Oct 29, 2023

volundmush commented Oct 29, 2023

Griatch left a comment

Choose a reason for hiding this comment

Griatch left a comment •

edited