Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add enforce_required_fields=False to records.decode_value, and callees. #10

Closed
wants to merge 1 commit into from

Conversation

ericvsmith
Copy link
Contributor

Add enforce_required_fields=False to records.decode_value, records.decode_record, and records.decode_records. See issue #9.

…code_record, and records.decode_records. See issue tdsmith#9.
@ericvsmith
Copy link
Contributor Author

@tdsmith : Can I get you to merge my 3 PRs (#7, #8, #10) and do a release? If not, I'm going to have to fork and put a copy on PyPI. Sorry for being pushy, but I've got a time constraint and the package needs to be on PyPI (not a private repo, long story). Thanks!

@tdsmith
Copy link
Owner

tdsmith commented Jan 6, 2024

Ah, it turns out I didn't have notifications enabled, thanks for the ping.

@tdsmith
Copy link
Owner

tdsmith commented Jan 7, 2024

This basically only works for string types, right? If there are specific fields that are impacted by this, I'd rather have the types on the models reflect the as-practiced convention rather than whatever the spec says, with an apologetic comment; I assume whatever ecosystem there is around this format needs to accommodate whatever Meet Manager does. Does that sound sensible?

@ericvsmith
Copy link
Contributor Author

At the point in decode_value() where the test is, value, and therefore stripped, is a string, right? So I think it would apply to all value types, since the conversions happen after this test.

I'm good with adding a flag to the model. I assume as another parameter to spec. I'm not sure what kind of name makes sense: I want to convey "MM says this is optional, even though the spec says it's required". Maybe just mm_optional? Or mm_not_provided? Let me know what you think and I can get it coded up.

Would you want to have both the new spec flag plus my existing enforce_required_fields parameter (possibly renamed)? I think so: I don't think we'd want to always allow "MM-laxness", but let me know your thoughts.

@tdsmith
Copy link
Owner

tdsmith commented Jan 7, 2024

Oh, that's right about other data types, sorry; I was reading too quickly.

I liked your proposal and landed an implementation in #11; let me know if you need something else!

@ericvsmith
Copy link
Contributor Author

I have not done extensive testing, but this looks good so far.

I have another wacky issue with Meet Manager and fractional result points (resulting from a tie). I'll open another issue.

I appreciate the help!

@ericvsmith ericvsmith closed this Jan 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants