Parsed field logic introduced #298
Conversation
K4liber
commented
Oct 19, 2025
K4liber
commented
- parsed field logic introduced
- IntegerField introduced
- related code adjusted
K4liber
force-pushed
the
feat/parsed_field_logic
branch
from
ef68145 to
9ba5d3c
Compare
seddonym
left a comment
seddonym
left a comment
There was a problem hiding this comment.
Thanks for submitting!
I like this, but I wonder if we need the .value API. Why not go one step further and just set the contract attribute to the parsed value, so we can access the value directly, e.g. contract.as_packages rather than contract.as_packages.value?
Incidentally, it has long bothered me that the current design isn't type safe - I do like the fact that your approach here means we no longer need # type: ignore, I have a feeling that change might require us to reintroduce it, so if you have any ideas for making the API clean and type safe, let me know.
|
|
||
|
|
||
| def _get_value_for_field(field: fields.Field[Any], value: Any) -> Any: | ||
| supported_field_types = (fields.BooleanField, fields.StringField, fields.IntegerField) |
There was a problem hiding this comment.
Rather than needing to keep track of the types that support this, why not have a default implementation method that these subclasses can override? Then we can add support just by overriding the method. What do you think?
There was a problem hiding this comment.
The issue is that it is not trivial to modify the current approach and have a clean typing for multiple value fields. So we have the artificial logic with a _ParsedField that allows to keep the current approach for multiple value fields just not to break it. The new approach with .value attribute introduce clean typing for a single value fields listed under supported_field_types variable.
Since contract fields are class attributes, some magic needs to be introduced to trick IDEs/type checkers to believe the attributes are just a builtin types. I don't have any idea of how to improve it without 1) introducing complex magic that will make it worse 2) completely changing the approach here.
If you think that the already proposed changes introduce more complexity than providing value, lets skip those changes. Maybe some day a new approach with a clean typing will arise.
There was a problem hiding this comment.
Hmm, ok. I do like the way this makes it type safe, but my concern is that strictly speaking the contract fields form a public API (via custom contracts). So while I'd like to change the way they're done, I'd like to do it all in one go - and this isn't quite there in its current form.
I think for this PR, could we stick with the existing pattern for the IntegerField (and not introduce this new _ParsedField approach? It's something I'd like to return to but don't want to let it distract from other work (such as the acyclic contract, which I would like to prioritize). Sound ok?
There was a problem hiding this comment.
Sure, I am also not convinced about the proposed solution. Let's wait for a better idea to come.
|
|
||
|
|
||
| def _get_value_for_field(field: fields.Field[Any], value: Any) -> Any: | ||
| supported_field_types = (fields.BooleanField, fields.StringField, fields.IntegerField) |
There was a problem hiding this comment.
Hmm, ok. I do like the way this makes it type safe, but my concern is that strictly speaking the contract fields form a public API (via custom contracts). So while I'd like to change the way they're done, I'd like to do it all in one go - and this isn't quite there in its current form.
I think for this PR, could we stick with the existing pattern for the IntegerField (and not introduce this new _ParsedField approach? It's something I'd like to return to but don't want to let it distract from other work (such as the acyclic contract, which I would like to prioritize). Sound ok?
2 participants
