Updated conversion functions

Official Announcements about Source.Python.
User avatar
Ayuto
Project Leader
Posts: 2193
Joined: Sat Jul 07, 2012 8:17 am
Location: Germany

Updated conversion functions

Postby Ayuto » Thu Nov 19, 2015 2:38 pm

We have updated all conversion functions. They now don't have the optional keyword argument "raise_exception" anymore (defaulting to True). Instead they only accept the value that should be converted and return the converted value. If the conversion failed, a ValueError will be raised.

Example:

Syntax: Select all

from players.helpers import index_from_userid

try:
index = index_from_userid(2)
except ValueError:
print('Failed to convert the userid into an index.')

We have also updated our listeners. Previously, they received an invalid value if the internal conversion failed. Now, the value is just None.

Example:

Syntax: Select all

from listeners import OnEntityCreated

@OnEntityCreated
def on_entity_created(index, entity):
if index is None:
# Internal conversion failed.
# The entity is probably a non-networked entity
print(entity.classname)
User avatar
Doldol
Senior Member
Posts: 200
Joined: Sat Jul 07, 2012 7:09 pm
Location: Belgium

Postby Doldol » Thu Nov 19, 2015 10:16 pm

I honestly really liked the try/except approach, this seems a bit less Pythonic, after all I would think that when I call a conversion function like this, I would expect to not encounter an error in the conversion process, making an exception more appropriate if an error still is encountered?

I'm sure this decision was carefully considered and the result is there for a reason, but I can't help but feel like I should point this out.
User avatar
Mahi
Senior Member
Posts: 236
Joined: Wed Aug 29, 2012 8:39 pm
Location: Finland

Postby Mahi » Thu Nov 19, 2015 10:28 pm

I think I'm with Doldol here, I'd prefer an error with try/except way more than getting None.
If I'm trying to convert an userid to an index and it fails, I expect and exception to be raised instead of getting None. It's not like int('hi') returns None either.

I agree that getting rid of the extra argument was good, but I think you should've gone the other route here: get rid of the None option and only use exceptions.
Just my opinion though.
User avatar
Ayuto
Project Leader
Posts: 2193
Joined: Sat Jul 07, 2012 8:17 am
Location: Germany

Postby Ayuto » Thu Nov 19, 2015 11:33 pm

The reason for this change was that we only wanted one behaviour and not two depending on the second argument.

To be honest, when I decided to return None I was a bit C/C++ driven, where it's common to return "false" on failure and "true" on success (the actual return value is passed as a reference through the function's parameters), and didn't really think of raising exceptions.

Are there more people voting for raising exceptions instead of returning None?
User avatar
Mahi
Senior Member
Posts: 236
Joined: Wed Aug 29, 2012 8:39 pm
Location: Finland

Postby Mahi » Thu Nov 19, 2015 11:37 pm

To be honest, when I decided to return None I was a bit C/C++ driven, where it's common to return "false" on failure and "true" on success
Good point, but I'd say we want to be pythonic here, not cppic!
User avatar
iPlayer
Developer
Posts: 590
Joined: Sat Nov 14, 2015 8:37 am
Location: Moscow
Contact:

Postby iPlayer » Fri Nov 20, 2015 8:44 am

I vote for exceptions.

If there was a possibility for user not having an index (but still valid userid), returning None would come in handy. But in this case returning None means that the calling script is working with invalid userid. I feel like this should be raised as early as it can be raised. Because if it's not, this None can be ignored by the script and then passed to another function that accepts both indexes and None as valid values, e.g. .take_damage's attacker_index or .call_input's caller. And instead of getting an error right away we get an unexpected behavior, bug that should later be located and fixed.

I remember ES that was returning empty strings and 0's here and there and that was giving a lot of headache.

EDIT: Also, if that raise_exception keyword was defaulting to True, what are the reasons to change the default behavior?
User avatar
Ayuto
Project Leader
Posts: 2193
Joined: Sat Jul 07, 2012 8:17 am
Location: Germany

Postby Ayuto » Fri Nov 20, 2015 2:44 pm

Alright, I have updated the conversion functions once again to raise an exception instead of returning None in case of a failed conversion. The main post has been updated as well.

Thank you for your feedback! :)
User avatar
Mahi
Senior Member
Posts: 236
Joined: Wed Aug 29, 2012 8:39 pm
Location: Finland

Postby Mahi » Fri Nov 20, 2015 4:35 pm

Ayuto wrote:Alright, I have updated the conversion functions once again to raise an exception instead of returning None in case of a failed conversion. The main post has been updated as well.

Thank you for your feedback! :)
Awesome work, looking good! :)

Thank you very much for all the effort!
User avatar
Doldol
Senior Member
Posts: 200
Joined: Sat Jul 07, 2012 7:09 pm
Location: Belgium

Postby Doldol » Sat Nov 21, 2015 1:27 am

Mahi wrote:
Ayuto wrote:Alright, I have updated the conversion functions once again to raise an exception instead of returning None in case of a failed conversion. The main post has been updated as well.

Thank you for your feedback! :)
Awesome work, looking good! :)

Thank you very much for all the effort!


Agreed! Thanks for being open and considering feedback!

Return to “News & Announcements”

Who is online

Users browsing this forum: No registered users and 17 guests