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

Ignore instead of forbid extra fields #89

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

MHHukiewitz
Copy link
Member

As the Extra.forbid flag has been a footgun to us for a long time, with no apparent reason (see What is Extra? and Why use Extra.forbid?), we should from now on simply use the default, Extra.ignore on our Pydantic models. This will allow forward-compatibility with newer message specs, but not impact existing functionality of old services using the aleph-message package.

@MHHukiewitz MHHukiewitz requested a review from hoh January 31, 2024 12:32
@hoh
Copy link
Member

hoh commented Feb 1, 2024

I have doubts whether this is the best approach to take 🤔

In particular, using this on pyaleph would allow users to publish messages now that might not match a future format, so it would not be forward compatible in that case.

@hoh hoh added the question Further information is requested label Feb 1, 2024
@MHHukiewitz
Copy link
Member Author

This appears to me then a footgun for people not using the correct message format and/or trying weird things with the API.

If this change means for us that we have an easier time upgrading the message spec, while avoiding breaking code of other users that upgrade their dependencies... Then I'd rather care to protect the latter group.

A solid versioning scheme might help avoid this dilemma alltogether, in the future. But as you mentioned once, we don't want to go to extreme lengths regarding this topic, at least for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants