Upgrade cel-python to 0.2.0 #252
Conversation
|
@jchadwick-buf it would appear that enabling typing in celpy hit off a bunch of type errors that look relatively valid to me — any chance you could take a peek and confirm it looks like something we should fix, rather than just disabling typing for the |
|
I'm weary of doing this, because I don't think cloud-custodian/cel-python#73 is fixed. We will have to carefully port our monkey patching in |
|
@jchadwick-buf hm, that may be an issue but I'm not sure it's relevant to the failures here - I'm mostly talking about the type mismatches that we seemingly have, e.g. in |
|
Yeah, I'm sure it's not related to the errors, but at the very least we can't unpin the cel-python dependency even if we update it. The code paths with errors are indeed exercised (by conformance), so I'm guessing the API or types have broken on us as of cel-python 0.2.0. |
The rest is just string_format, which does not appear to be flexed by conformance.
|
@jchadwick-buf I took a pass at updating some of the types in 9e83b45. The only remaining errors are in StringFormat, which AFAICT is not flexed by the conformance tests. (Should it be?)
I guess I'm confused — the tests here, including the conformance tests, all pass — the only thing that doesn't work are the type-hints, which are only new due to celpy enabling them. I think the latest commit is correct for the types I changed, and now I'd just like to determine if we want to fix the remaining typing issues with StringFormat (which, to be clear, I think are real runtime issues because nothing I can see is running that code). Isn't your monkey-patching in |
|
My concern is that if our dependency states we can take any version of cel-python, then presumably when someone depends on us, they can get a newer version of cel-python that we haven't run the conformance tests against. |
Sorry I should have been clear — that's what this PR is doing: https://github.com/bufbuild/protovalidate-python/pull/252/files#diff-a86c67a0a29ed0e95909b9b7c420140f302d17399ee6dcce4e1a51a14d27fd51R26. (I had to remove the direct pin on |
|
Interesting. Sorry, I didn't realize it couldn't just be updated by changing the version number in the Pipfile. |
|
yeah, bumped the version and then re-locked the Thoughts on fixing StringFormat? It looks like it must be available in the CEL env and I'm assuming it's tested in our other impls, but doesn't seem to be tested here AFAICS? |
|
String formatting is used extensively in protovalidate, e.g. it is used by many of the standard rules. (For example, look at this.) It definitely will get exercised by the conformance test. |
It would seem that the conformance tests do not flex the formatting that we appear to support here, or just don't check for it?
stefanvanburen
force-pushed
the
svanburen/upgrade-cel-python
branch
from
f9c905c to
3071c6d
Compare
yeah, interesting — I think maybe those tests just aren't flexing whatever we're providing as a format implementation, as simplifying the implementation of |
|
ah, I think I might see what's happening: we aren't passing |
This reverts commit 3071c6d.
Looks like we're just a little off in a few places.
| return celpy.native_to_cel(celpy.new_error("format() requires a string as the first argument")) | ||
| return celpy.CELEvalError("format() requires a string as the first argument") | ||
| if not isinstance(args, celtypes.ListType): | ||
| return celpy.native_to_cel(celpy.new_error("format() requires a list as the second argument")) | ||
| return celpy.CELEvalError("format() requires a list as the second argument") |
There was a problem hiding this comment.
I didn't see a native_to_cel or new_error anywhere in cel-python — while I doubt we ever hit these cases (more of an assert?), figured it made sense to update them.
protovalidate/internal/field_path.py
Outdated
There was a problem hiding this comment.
Unused code AFAICT.
There was a problem hiding this comment.
Although this is technically unused (after removing the legacy field path field), we actually plan on exposing it as a public API.
There was a problem hiding this comment.
ah, ok. want me to add it back here, or are you going to add it in some other form pre-1.0?
There was a problem hiding this comment.
no big deal, it's always in the version control history
|
@jchadwick-buf I think this is ready for a look when you've got review cycles, but no rush. |
Ref: https://github.com/cloud-custodian/cel-python/releases/tag/v0.2
Relates to #180.