Fix bug in state redaction and add full test coverage for redact.py

[steinmn]

Bug description:
The key input to the redaction became "950 (MacMain)", which did not match the REDACT_KEYS entry "MacMain", so the value was not redacted.

[sveinse]

I've been sitting on an improved version of redact.py for a while. The file itself was complete, but I was lacking proper tests and there it stopped. May I suggest that we merge these two into this PR?

sveinse@4499078

[sveinse]

Good catch on this issue. However, I don't think we have a complete fix for the issue. keyv might be None if the obj[key] is not listed in ZCONST.observations ¹, L160. So what should the key= argument be when keyv is None? Perhaps:

obj[value] = self(obj[value], key=keyv or "", ctx=ctx)

key is kind of useless without a suitable string in keyv.

Footnotes

1. I really dislike that the ZCONST.observations is s two-way lookup; int->str and str->int. It's a nightmare for type annotations and it should be refactored With this setup keyv might also return an int if obj[key] happens to be a str. ↩

[steinmn]

Isn't that case caught by L163? Or does None not in self.REDACT_KEYS evaluate to false? Definitely could be made more explicit though...

[sveinse]

Yes, you're right. This also means that if the keyv is not in self.REDACT_KEYS the value will never be checked for redactable content, which is possibly an oversight. Perhaps add a test for that case?

[sveinse]

I think maybe the code can be something like this:

        for obj in objs:
            for key in self.OBS_KEYS:
                if key not in obj:
                    continue

                # Get the string for the observation key
                keyv: str = ZCONST.observations.get(obj[key], "")
                if keyv:
                    # Write the observation string name into the state object
                    obj[key] = f"{obj[key]} ({keyv})"

                # Redact the value if needed
                for value in self.VALUES:
                    if value not in obj:
                        continue
                    obj[value] = self(obj[value], key=keyv, ctx=ctx)

Note that I haven't tested this. A test case should be written to verify the intended behavior.

This code yields an type checking error because of ZCONST.observations dual type. I'm not sure right now how it's best to deal with that duality without setting up more run-time checks.

[steinmn]

A test case should be written to verify the intended behavior.

Done, and code works as intended. Not often I get to do actual test-driven-development and write the test before the fix, but every time I do, it feels really nice.

Sidenote: Apparently you don't get the __getitem__-function until you get to Sequence, so I swapped Iterable for that in order to avoid lots of type checking errors in the test file.

[steinmn]

This code yields an type checking error because of ZCONST.observations dual type. I'm not sure right now how it's best to deal with that duality without setting up more run-time checks.

Create a new issue called "Refactor ZCONST.observations" where you describe the problems with two-way lookup/typing?

[sveinse]

Done. See #341 and #342

[steinmn]

May I suggest that we merge these two into this PR?

Sure, maybe merge the pydantic-update into master first so we can rebase on that and not have the validate-manifest-test failing?