opendp / dp-wizard

Web application that makes data releases that satisfy differential privacy using the OpenDP Library
MIT License
0 stars 1 forks source link

Type checking with pyright #164

Closed mccalluc closed 4 days ago

mccalluc commented 1 week ago

My plan is to continue this work incrementally, but I think having the checks in place is enough for now. I would particularly like to clean up a couple places where dicts could be replaced with NamedTuples.

For the reviewer:

mccalluc commented 5 days ago

After sitting with this, I think my preference would be to remove strict, and then most of the overrides could be removed, but it would be good to get someone else's opinion. That way, we would get the benefit of the checks where they are supplied, and it would be up to us to keep adding them on new code going forward.

ChengShi-1 commented 5 days ago

After sitting with this, I think my preference would be to remove strict, and then most of the overrides could be removed, but it would be good to get someone else's opinion. That way, we would get the benefit of the checks where they are supplied, and it would be up to us to keep adding them on new code going forward.

I agree with opting for non-strict mode, as this doesn't appear to be a complex application. Removing strict could make the codebase cleaner and enable faster development while still let us to selectively apply strict checks where needed.

mccalluc commented 4 days ago

Let me merge main into this and we can go from there.

Also, it seems like a bunch of warning happened because the Reactive.Value[] type was not equal to primitive type even though they are both int/float/other.

These reactive.values do need to be treated differently from the types they wrap, and having a warning about these is actually one of the big wins. To get the wrapped value from a reactive.value, you need to call it as a function:

>>> name = reactive.value('chuck')
>>> name
<function name at 0x...> or something like that
>>> name()
'chuck'
>>> name.set('not chuck')
>>> name()
'not chuck'

(This is not an actual repl session: Reactive values don't work outside of a reactive context, so this wouldn't work.)

mccalluc commented 4 days ago

Tidied up after merge from main, and applied typing with the nice NamedTuples that were introduced in the refactoring of code generation. Merging this much will be a big improvement, and we can still make incremental improvements.