Remove mutable default types.

[npbaskerville]

Simply replacing any instances of mutable default args with immutables. These were only in ib.py and only lists, which have all just been replaced with tuples.

[mattsta]

Thanks for the improvements!

Unfortunately, the other notes in the original reply seem to have been missed: #81

The source branch is next for updates and I'm fairly certain empty tuples would break the network protocol because the protocol writer only accepts lists the container type here:

ib_async/ib_async/client.py

Lines 254 to 301 in dab8622

	# fmt: off
	FORMAT_HANDLERS: dict[Any, Callable[[Any], str]] = {
	# Contracts are formatted in IBKR null delimiter format
	Contract: lambda c: "\0".join([
	str(f)
	for f in (
	c.conId,
	c.symbol,
	c.secType,
	c.lastTradeDateOrContractMonth,
	c.strike,
	c.right,
	c.multiplier,
	c.exchange,
	c.primaryExchange,
	c.currency,
	c.localSymbol,
	c.tradingClass,
	)
	]),
	# Float conversion has 3 stages:
	# - Convert 'IBKR unset' double to empty (if requested)
	# - Convert infinity to 'Infinite' string (if appropriate)
	# - else, convert float to string normally
	float: lambda f: ""
	if (makeEmpty and f == UNSET_DOUBLE)
	else ("Infinite" if (f == math.inf) else str(f)),
	# Int conversion has 2 stages:
	# - Convert 'IBKR unset' to empty (if requested)
	# - else, convert int to string normally
	int: lambda f: "" if makeEmpty and f == UNSET_INTEGER else str(f),
	# None is always just an empty string.
	# (due to a quirk of Python, 'type(None)' is how you properly generate the NoneType value)
	type(None): lambda _: "",
	# Strings are always strings
	str: lambda s: s,
	# Bools become strings "1" or "0"
	bool: lambda b: "1" if b else "0",
	# Lists of tags become semicolon-appended KV pairs
	list: lambda lst: "".join([f"{v.tag}={v.value};" for v in lst]),
	}
	# fmt: on

[npbaskerville]

Yes, sorry, missed the comment about the target branch.

I've updated it to use None as the default, but I had also taken care to convert the incoming tuples to lists anyway and I've retained those changes here. If, as you say, there is some critical dependence on the inputs' being List[T] not just Sequence[T], then we shouldn't trust that but convert.

I'm fairly certain empty tuples would break the network protocol because the protocol writer only accepts lists the container type here

Interesting. Would be keen to understand this better, as it sounds like a there's a separate issue here to make the code more robust. In the particular case of the client.py code you linked to (thanks), it would be easy enough to update FORMAT_HANDLERS to match on general sequences.

[pavel-slama]

@npbaskerville Hello, I'm not a maintainer here, but I use the library and am long time SW dev. Got few notes..

Could you use list[whatever] | None instead Optional[List[whatever]]?
It's preferred way since python 3.10 (note lower case "l" in "list") - it's more readable and doesn't need any import from typing. Just in case someone would run this with older python, you can add at the top:
from __future__ import annotations

I guess it's a matter of taste but I'd remove type casts to list - sending wrong type is a bug in calling code that the cast will hide from fixing

[npbaskerville]

Okay, I checked the mypy docs and I am indeed out of date on the Optional best practice - I will update the PR later. Thanks.

sending wrong type is a bug in calling code that the cast will hide from fixing

My objection here is that I do not believe list[T] is the correct type for any of these methods or functions. There is nothing about the code that meaningfully requires a list as opposed to a general Sequence, it's just that it's currently written in such a way that it has to be a list due to the FORMAT_HANDLERS keys. Even the actual formatting code here would work equivalently with any Sequence. Personally, I don't think I've typed an input arg as a list.

Happy to cede to the maintainers on in-house style here though.

[pavel-slama]

Agreed that there's probably no need for lists (IMHO it doesn't even have to be sequence for what it's worth, just iterable is fine), but I'm not that versed with actual IB API protocol.

But I'd hesitate making any substantial changes until this whole thing is properly covered with tests.

[npbaskerville]

I have update the Optional to T | None

IB API protocol

Doesn't look like that's important. This code is just iterating through the list and creating a string.

But I'd hesitate making any substantial changes until this whole thing is properly covered with tests.

I share your hesitation. That's why I'd suggest keeping the list conversion I added.

[mattsta]

So.......

Lots of crossed wires here:

• syntax is still kinda broken (the List[]) syntax is 3 years old and we don't use it — and in fact these changes reverted modern syntax for outdated syntax)
• Optional is also outdated.
• We don't want to introduce any additional conversions or checks if they don't add any user benefit (e.g. extra "convert arbitrary iterables (could be a sequence or a set or a generator or anything) to lists" doesn't make anything faster or better for users). Sure, it's not a lot of extra overhead, but unnecessary overhead is still unnecessary when things currently work. So adjusting types is fine, but rewriting types then adding converters when they weren't previously necessary isn't the best for users.

[npbaskerville]

syntax is still kinda broken (the List[]) syntax is 3 years old and we don't use it — and in fact these changes reverted modern syntax for outdated syntax)

I reverted that change yesterday in this commit.

Optional is also outdated.

ditto

We don't want to introduce any additional conversions or checks if they don't add any user benefit (e.g. extra "convert arbitrary iterables (could be a sequence or a set or a generator or anything) to lists" doesn't make anything faster or better for users). Sure, it's not a lot of extra overhead, but unnecessary overhead is still unnecessary when things currently work. So adjusting types is fine, but rewriting types then adding converters when they weren't previously necessary isn't the best for users.

I have now just removed the list conversion. The change was intended to make the code less brittle and avoid headaches later on.