General code improvements/refactoring

[lojack5]

This is meant to be a tracker for general code improvements that aren't big enough issues to warrant their own issue, and not small enough to just make the changes directly. Most of these initial issues are thoughts I had while writing type-stubs and noticing places for improvements.

Tick off items if they've been addressed. If we decided to do nothing about it, additionally mark it as such by striking it through.

• [ ] bosh.Omod and bosh.OmodConfig both handle reading and writing omod config files. Consider merging the interfaces into a common one?
• [ ] Contants in env can be replaced with enums:
  • [ ] FO_* constants with an IntEnum.
  • [ ] FOF_* constants with an IntFlags..
• [ ] bosh.SaveFileHeader: Multiple methods are only called internally, and all are called somewhere in a call chain leading back to __init__. Also, many object attributes are not set until these are called. Consider privating these methods, load_masters at the very least.
  • [x] load_masters: Call chain: __init__ -> read_save_header -> load_header -> load_masters + _decode_masters. This one in particular leaves the masters attribute in an inconsistent state if called externally without also calling _decode_masters.
  • [ ] load_header: Call chain: __init__ -> read_save_header -> load_header.
  • [ ] calc_time: Call chain: __init__ -> read_save_header -> load_header -> calc_time.
  • [ ] load_image_data: Call chain: __init__ -> read_save_header -> load_header -> load_image_data.
• [ ] bolt: unpack_str_int_delim, unpack_str_byte_delim, and unpack_str16_delim naming is confusing. The first two return a size delimiter, while the last returns the string.
• [x] bolt.Flags - replace with enum.IntFlag?. As far as I can tell it should be pretty much a drop in replacement, just have to deal with from_names usages. IntFlag doesn't provide a nice interface for setting values on enums to true/false nicely. Still, could upgrade bolt.Flags to have a nicer interface for defining the flag names in a way type-checkers can understand.
• [ ] bosh.cosaves.ACosave - make an enum for loading_state.

[Utumno]

There is a big discussion on Flags in #480 + a few merges like 87c7a4cc66bddfc4d18511023e4dc41633c86043 - conclusion was that indeed IntFlag can't serve as a drop in replacement

[lojack5]

Yeah, the biggest stopper that I ran into while investigating is that IntFlags are ints, which are an immutable type. Pretty much prevents us from every providing a flags.isHidden = True interface. The best we could get is compound assignment operators, flags |= flags.isHidden. And once py 3.11 comes we're loosing access to the flag enums on instances, so that would have to change to flags |= FlagClass.isHidden, and testing for truthyness would have to change to if FlagClass.isHidden in flags:. Overall I like the mutableness of our flags. My Idea though is we might be able to implement some metaclass magicry to make definitions easier, like:

class _MarkerFlags(Flags):
    visible: bool
    can_travel_to: bool
    show_all_hidden: bool

That'd take some work to get right though, messing with metaclasses has all sorts of pitfalls, and doing it in a way so typecheckers see the flag names as bools while also being able to define them the way you want is even weirder.

[Utumno]

Re: flags yes - see also remarks in 87c7a4cc66bddfc4d18511023e4dc41633c86043. Bottom line those are better left alone for now.

I will be adding some todos that I run across in 543 (that is anywhere) in this issue that you might be interested into - it is especially around converters on which I had to plunge as I am trying to finalize 543 - there is some pretty complex code in there:

• [ ] __extractOmods vs _extractOmods 1987c08fa978437daf7bf5b775937d58cd73e327 - don't underestimate this one
• [ ] drop refreshConvertersNeeded - see 56b0d76cc2defeb10deb3efa97fc0cfe6380d331
  • [ ] also drop _prune_converters
• [ ] ##: Needed? Shouldn't this be handled by cached_is_active? seems not needed indeed -> b887c84b8ecad1e1d1652915cac8dde88cb67c49