Fixing bug in decoder eval for optional fields

[rpmcginty]

Issue: #353

Problem

There is inconsistent behavior in how a custom decoder is used depending on whether the field of the custom decoder has Optional type.

For optional fields which can be None, you are supposed to use the Optional[...] type. However, when doing so, it seems that custom decoders are always evaluated.

It should be noted that fields where the default value is None but no Optional[...] typing is present, a custom decoder will only be used if a non-None value is provided.

Solution

Modified decoder function s.t. when field value is None, both cases where field type is not optional (existing logic) as well as where field type is optional are handled.

[rpmcginty]

@lidatong is there any concern with this change?

[rpmcginty]

@lidatong is there a timeline for merging into the package?

[TristanSpeakEasy]

@lidatong we have run into this issue as well, can we get this merged and a new release created?

[simplesagar]

@lidatong Ran into this issue as well. Would love to know if its getting merged soon :)

[patricksnape]

Whilst we are piling on - I would also love to see this merged, this causes very surprising results when a field is constructed incorrectly by passing None where None is not valid (you essentially just get a warning and then are provided with an invalid class back.

[TristanSpeakEasy]

Unfortunately it seems that this project is likely abandoned, we have been maintaining a separate fork for our own use-case but would love to see this repo get some love @lidatong or a new maintainer take up the reigns (which might need to be via a fork of this if the original author doesn't respond)

[lidatong]

Unfortunately it seems that this project is likely abandoned, we have been maintaining a separate fork for our own use-case but would love to see this repo get some love @lidatong or a new maintainer take up the reigns (which might need to be via a fork of this if the original author doesn't respond)

hi, i'm really sorry for the long delay. it's been a combination of work stress and burnout, but that is no excuse for my procrastination. i do really appreciate PRs from the community and will try to review and merge the high-demand ones this weekend! i'm also open to granting write permissions to repeat contributors, to make this project more sustainable

[rpmcginty]

Is there an option for the latter?

[rpmcginty]

@lidatong Let us know if granting wire permissions are an option. It would be good to get a community to help you out with making updates to this really useful package

[lidatong]

@lidatong Let us know if granting wire permissions are an option. It would be good to get a community to help you out with making updates to this really useful package

sorry, i just fixed CI issues. definitely can grant write, though publishing is a final step i'll aim to not bottleneck -- it would be good to for to review all changes anyways before signing anything

[matt035343]

Also, please sync your branch to latest master commit. Then we can proceed with a merge

[rpmcginty]

@matt035343 I have rebased and fixed formatting errors

[george-zubrienko]

Unfortunately it seems that this project is likely abandoned, we have been maintaining a separate fork for our own use-case but would love to see this repo get some love @lidatong or a new maintainer take up the reigns (which might need to be via a fork of this if the original author doesn't respond)

No stress, there are extra hands on deck now :) please let me or @s-vitaliy or @matt035343 know if there are other PRs you want reviewed asap