Improved game._is_action_allowed()

[mrbermell]

In game.py, the method _is_action_allowed() is renamed to is_action_allowed() to make it public.

I've also removed the print statements, which are annoying and confusing. Instead the function returns an object that can be evaluated as bool but also contain an error message which is useful when debugging for example, example usage:

if not game.is_action_allowed(action):  # can be evaluated as bool 
    print(f"{game.is_action_allowed(action)}")  # can be evaluated as string, will print error message with helpful detail

Let me know what you think!

[njustesen]

I think renaming the function to make it public is a good idea but I'd prefer not to do it now. Let's wait for after Bot Bowl IV.

Not sure I am a fan of the BoolWithMsg. There must be a more standard way to solve this issue. What about warnings or maybe make use of the config.debug flag?

[mrbermell]

I think renaming the function to make it public is a good idea but I'd prefer not to do it now. Let's wait for after Bot Bowl IV.

Yes, let's wait.

Not sure I am a fan of the BoolWithMsg. There must be a more standard way to solve this issue. What about warnings or maybe make use of the config.debug flag?

Having thought about it, we should just have two functions. I propose this, what do you think?

def is_action_legal(action) -> bool: 
   ... 

def get_illegal_action_warning(action) -> str: 
    ... 

[njustesen]

When and how would you use get_illegal_action_warning(action) -> str:?
Is it better than just keeping the warnings and only raise then if config.debug = True

[mrbermell]

When and how would you use get_illegal_action_warning(action) -> str:? Is it better than just keeping the warnings and only raise then if config.debug = True

I'll try to make my case with three arguments.

1. You'll only want to get the warning text when you're debugging some error. But if you're using is_action_allowed as part of some logic, as is done in examples/scripted_bot.py:

# Execute planned actions if any
while len(self.actions) > 0:
    action = self._get_next_action()
    if game._is_action_allowed(action):
        return action

In the above code it's fair to say that you are not interested in the warnings. If you enable debug here, you'll likely have many warnings spammed in the prompt. But you are interested in the warnings in some other part of the code. Inconvenient sorting through the error messages, can you always trust that it's always the last?

2. The debug flag enables a lot of other output that's probably not interesting.

3. is_action_allowed should be considered a user function (since it's used in our example code). And you can't tell that this is where you'd find the error message by the function name. I like making it explicit.