chore: Issue warning announcing entity's value_type as mandatory - #4833
Conversation
Sorry, something went wrong.
- Add deprecation warning when value_type is not specified for an entity - Add test cases to verify deprecation warning behavior - Prepare for making value_type mandatory in next release Issue: feast-dev#4670 Co-Authored-By: Francisco Javier Arceo <arceofrancisco@gmail.com> Signed-off-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
- Reorder imports according to PEP8 - Group standard library imports together - Fix ruff linting issues Co-Authored-By: Francisco Javier Arceo <arceofrancisco@gmail.com> Signed-off-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
| warnings.warn( | ||
| "Entity value_type will be mandatory in the next release. " | ||
| "Please specify a value_type for entity '%s'." % name, | ||
| DeprecationWarning, |
There was a problem hiding this comment.
why not FutureWarning instead?
https://docs.python.org/3/library/exceptions.html#FutureWarning
Sorry, something went wrong.
There was a problem hiding this comment.
DeprecationWarning:
Base class for warnings about deprecated features when those warnings are intended for other Python developers.
Ignored by the default warning filters, except in the main module (PEP 565). Enabling the Python Development Mode shows this warning.
FutureWarning
Base class for warnings about deprecated features when those warnings are intended for end users of applications that are written in Python.
I think DeprecationWarning is a reasonable choice but I could see both ways. Devin chose the former. 🤷
Sorry, something went wrong.
There was a problem hiding this comment.
I can’t see any deprecation actually, but if Devin will become the main maintainer, that’s fine 🤷🏻♂️
Sorry, something went wrong.
There was a problem hiding this comment.
I'm going to force it next release.
Sorry, something went wrong.
What this PR does / why we need it:
Make value_type for Entity issue a warning if not set. Next release we'll make it mandatory.
Which issue(s) this PR fixes:
#4670
Misc