Refactor UUID cache

[hephaisto]

@ubruhin : Would this kind of refactoring be okay? If yes, I will update the other generators accordingly.

[ubruhin]

I think this looks very good and will make the scripts cleaner 👍

Only two thoughts so far:

• There exist places where the UUID cache is used read-only (e.g. to get generic connector UUIDs from the JST generator, see here). For those cases, I think we should have an option to use UuidCache without a with statement, and without the stale check. Maybe an optional parameter open: bool = False in the constructor of UuidCache (open=True would load the cache immediately)?
• Somehow I don't like the additional indentation level in __main__() - many of those functions are already hard to read/format due to tons of data for all the part variants. I wonder if we should just rename __main__() to main() and call it from a short wrapper which does the with uuid_cache: main()? The CLI argument handling may also be part of this wrapper then.

[hephaisto]

I already thought about making a main() function because having all the stuff in the __main__ is quite dirty (everything in there goes into the global namespace). I will look how to do that better.
I think extracting the CLI handling will overload this PR.

I could:

• 1️⃣ extract opening to an open() method which can be called directly.
• 2️⃣ add a save=True argument which skips saving. Would be cleaner/more consistent in my opinion.

[ubruhin]

1️⃣ extract opening to an open() method which can be called directly.

Hmm but then we have to make an extra call for that:

uuid_cache = UuidCache('uuid_cache_axial_tht.csv')
uuid_cache_connectors = UuidCache('uuid_cache_connectors.csv')
uuid_cache_connectors.open()  # either here

def main():
    uuid_cache_connectors.open()  # or here
    with uuid_cache:
        # ...

2️⃣ add a save=True argument which skips saving. Would be cleaner/more consistent in my opinion.

I think a flag for the read-only mode makes more sense because it can/should have more features:

• Load the file immediately, no need for with statement
• Skip saving (so we also don't need the with statement technically)
• Also skip stale check since this doesn't make sense when just reading the cache of another generator
• Make get() failing if a key doesn't exist (instead of creating a new key). That is actually an important behavior for generators which access the UUID cache of other generators - accessing a nonexistent key is an error in that case.

IMHO we could even distinguish between those two modes (read-only vs. read-modify-write) by the usage of the with statement:

• The constructor always tries to load the cache, since this is always needed (no need to wait for __enter__()).
• If __enter__() is invoked, a flag like self.writable = True is set.
• If get() is called, the behavior depends on that flag:
  • self.writable==False: An exception is thrown if the key doesn't exist
  • self.writable==True: A new key is inserted if it doesn't exist yet
• In __exit__(), the cache is saved and checked for stale entries, since this is only called in writable mode

Though this distinguishing is neither explicit nor obvious, so I understand if you consider this a bad idea 😅 My alternative suggestion would be just to pass read_only=True to the constructor, so it would be explicit and still easy to use:

uuid_cache = UuidCache('uuid_cache_axial_tht.csv')
uuid_cache_connectors = UuidCache('uuid_cache_connectors.csv', read_only=True)
# read_only=True: open cache immediately, no stale check, no saving, exception when accessing nonexistent key

def __main__():
    with uuid_cache:
        # ...

I think I'd still prefer the implicit mode, since then we cannot forget to pass read_only=True:

uuid_cache = UuidCache('uuid_cache_axial_tht.csv')  # opened read-only
uuid_cache_connectors = UuidCache('uuid_cache_connectors.csv')  # opened read-only

def __main__():
    with uuid_cache:  # automatically switches to writable mode
        # ...

Anyway, I'm open for other ideas which cover those two use-cases in a simple-to-use way.

[ubruhin]

Just a side note: As long as we set up that global UuidCache object which gets "initialized" in __main__(), but then accessed directly from any freestanding function through the global object, the solution will be ugly anyway IMHO. Therefore I think another ugly behavior like the implicit detection doesn't make the whole thing much worse. In the end, this is not end-user software, it is just a helper tool for devs which is used from time to time, therefore IMO we don't have to make it very beautiful - I consider an easy-to-use, pragmatic API more important here.

[ubruhin]

Thanks for the update. From looking at the code, I'd say this is good 👍

So the remaining generators can be updated - hopefully the new concept works as intended ;-)

[ubruhin]

No longer needed

Suggested change

assert self.data is not None, 'Exiting non-entered generator'

[ubruhin]

Hmm this function is now just a plain wrapper without any additional functionality. IMO it should be removed then - up to you, no strong opinion.