Tweak type error handling #12
Conversation
seddonym
force-pushed
the
tweak-type-error-handling
branch
from
abc6f48
to
d55babe
Compare
.pre-commit-config.yaml
Outdated
There was a problem hiding this comment.
Thought (out of scope): Are we running pre-commit in CI? If not, perhaps we should be.
There was a problem hiding this comment.
Oh interesting, I hadn't thought of that as being standard practice, but maybe it's a good idea. Do we do that on kraken-core?
There was a problem hiding this comment.
(We are running the same things that precommit does in CI, though, such as linting and formatting checks.)
There was a problem hiding this comment.
It's not uncommon to use pre-commit to do the running of those tools, especially in smaller projects.
KC does not do that.
tests/test_python_interface.py
Outdated
| ( | ||
| object(), | ||
| 34.3, | ||
| 1_000_000_000_000, # Larger than unsigned long integer. |
There was a problem hiding this comment.
Suggestion: It looks like ints are unacceptable, regardless of if they're in range. Perhaps we should add that to the test too.
(Perhaps it's just not clear to me why we're testing out-of-range values in this way.)
There was a problem hiding this comment.
Oh that's a good point - the out of range thing is only relevant for the variable values. I'll change it.
tests/test_python_interface.py
Outdated
| 34.3, | ||
| 1_000_000_000_000, # Larger than unsigned long integer. |
There was a problem hiding this comment.
Similar to the comment above, it might be worth accounting for ints that are in range here too.
There was a problem hiding this comment.
That's covered by the test below, test_selector.
But I just realised that this should say 'signed', not 'unsigned', so thanks for drawing my attention to it!
| if !python_key.is_instance_of::<PyString>() { | ||
| return Err(PyTypeError::new_err(format!( |
There was a problem hiding this comment.
Thought (out of scope): I'm beginning to wonder if we might be able to represent this whole for-loop-context with a match statement? 🤔
There was a problem hiding this comment.
Yeah, there's certainly room for improvement! I'm still a noob at Rust. 😊
In a moment we will change the behaviour, and it'll be easier to see from one test.
seddonym
force-pushed
the
tweak-type-error-handling
branch
from
d55babe
to
cc4be14
Compare
Tweaks the way we handle problematic types passed to
bundle.get_translation, relating to fluent variables.TypeError. Variable keys should be hardcoded in an application, so it's helpful to see it crash.