Replace assigned lambdas with functions

[Solomon1732]

Platform

All; asyncio branch;
https://github.com/Tribler/py-ipv8/blob/asyncio/ipv8/util.py#L12

Expected behavior

Write functions instead of lambdas.

Actual behavior

Lambdas are assigned to variables, mooting the point of having them as "anonymous" functions.

Instead of

cast_to_unicode = lambda x: "".join([chr(c) for c in x]) if isinstance(x, bytes) else str(x)
cast_to_bin = lambda x: x if isinstance(x, bytes) else bytes([ord(c) for c in x])
cast_to_chr = lambda x: "".join([chr(c) for c in x])
old_round = lambda x: float(math.floor((x) + math.copysign(0.5, x)))

They can be instead written as

def cast_to_unicode(obj):
    return "".join(chr(c) for c in obj) if isinstance(obj, bytes) else str(obj)


def cast_to_bin(obj):
    return obj if isinstance(obj, bytes) else bytes(ord(c) in obj)


def cast_to_chr(obj):
    return "".join(chr(c) for c in obj)


def old_round(num):
    return float(math.floor((x) + math.copysign(0.5, num)))

[Solomon1732]

Another thought that occurred to me is that functions can be split to be single-dispatch functions (though it may be an overkill in this case):
https://docs.python.org/3/library/functools.html#functools.singledispatch

The upside is that it would remove the need to implement single dispatch manually via isinstance, and would make room for more specific type optimizations. The downside is more stuff to write and all that comes with it.

[qstokkink]

Good point, lambdas are also a bit slower than function definitions.

I don't really see the point of using singledispatch here though. However, I can also not find a proper comparison of the performance cost of using singledispatch, if it is faster it would make sense.

[Solomon1732]

As far as performance goes we can play it safe here and not use singledispatch, and refractor it only when the need arises. What do you think?

[Solomon1732]

I see old round is used only in one place. Looks like it can be removed. Is the functionality of it still needed?
https://github.com/Tribler/py-ipv8/blob/asyncio/ipv8/attestation/trustchain/block.py#L108

[Solomon1732]

Another thing I can think about is whether the lambdas are even needed. The conversation of Tribler/tribler#5023 made me think whether they're necessary.

[qstokkink]

I see old round is used only in one place. Looks like it can be removed. Is the functionality of it still needed?
https://github.com/Tribler/py-ipv8/blob/asyncio/ipv8/attestation/trustchain/block.py#L108

This concerns the blockchain block format. In the past, we used to serialize and unserialize the block itself before checking its signature. Now that we no longer do this, it should be fine to remove the old round. I don't believe any of our upstream projects use this either.

[qstokkink]

Another thing I can think about is whether the lambdas are even needed. The conversation of Tribler/tribler#5023 made me think whether they're necessary.

Well, strictly speaking, lambda functions are never necessary. Sometimes it's just more readable to have one-liners than micro-functions, which take more space. Also, people with functional programming backgrounds love them.

[qstokkink]

As far as performance goes we can play it safe here and not use singledispatch, and refractor it only when the need arises. What do you think?

I agree.

[Solomon1732]

Well, strictly speaking, lambda functions are never necessary. Sometimes it's just more readable to have one-liners than micro-functions, which take more space. Also, people with functional programming backgrounds love them.

Oh, I mean which functionality is necessary. For example in the PR I mentioned it turned out the functionality of cast_to_unicode_utf8 was no longer necessary since it was a py2-to-py3 conversion utility, used only in one place, and that place uses strings anyway (which are of course utf8 in py 3).

Now that we no longer do this, it should be fine to remove the old round. I don't believe any of our upstream projects use this either.

GitHub\tribler> rg 'old_round\('
Tribler\anydex\anydex\core\orderbook.py
51:            delay = int(ask.timestamp) + int(ask.timeout) * 1000 - int(old_round(time.time() * 1000))
70:            delay = int(bid.timestamp) + int(bid.timeout) * 1000 - int(old_round(time.time() * 1000))

Tribler\anydex\anydex\core\tick.py
164:               int(old_round(time.time() * 1000)) >= int(self.timestamp) - self.TIME_TOLERANCE and \

Tribler\anydex\anydex\core\timestamp.py
34:        return cls(int(old_round(time.time() * 1000)))

Tribler\anydex\anydex\core\timeout.py
34:        return int(old_round(time.time() * 1000)) - int(timestamp) >= self._timeout * 1000

Tribler\anydex\anydex\test\core\test_order.py
45:                            Timeout(5), Timestamp(int(old_round(time.time() * 1000)) - 1000 * 1000), True)

Tribler\anydex\anydex\test\core\test_timestamp.py
27:        self.assertAlmostEqual(int(old_round(time.time() * 1000)), int(Timestamp.now()), delta=1000)

Tribler\anydex\anydex\test\core\test_timeout.py
27:        self.assertTrue(self.timeout1.is_timed_out(Timestamp(int(old_round(time.time() * 1000)) - 3700 * 1000)))
28:        self.assertFalse(self.timeout2.is_timed_out(Timestamp(int(old_round(time.time() * 1000)))))

Tribler\Test\GUI\FakeTriblerAPI\models\channel.py
23:        self.timestamp = int(old_round(time.time())) - randint(0, 3600 * 24 * 7)

Tribler\pyipv8\ipv8\attestation\trustchain\block.py
108:            self.timestamp = int(old_round(time.time() * 1000))

Tribler\anydex\pyipv8\ipv8\attestation\trustchain\block.py
108:            self.timestamp = int(old_round(time.time() * 1000))

Seems like mainly anydex uses it, and Tribler in one of the tests. At least from a short check with ripgrep. Should int be used directly in these places instead of passing the result through oldround?

[qstokkink]

Should int be used directly in these places instead of passing the result through oldround?

Yes; I believe that is safe to do now.

[Solomon1732]

I'll open parallel issues in anydex and Tribler just in case. At worst is it will stay as a function.

[Solomon1732]

Since the parallel issues are in favor of removing old_round I'll be removing it.

[qstokkink]

Closed by #652