-
Notifications
You must be signed in to change notification settings - Fork 39
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 DeprecatedFIeld for more informative procedure for deprecating fields of artifacts #741
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #741 +/- ##
=======================================
Coverage ? 91.13%
=======================================
Files ? 98
Lines ? 9897
Branches ? 0
=======================================
Hits ? 9020
Misses ? 877
Partials ? 0 ☔ View full report in Codecov by Sentry. |
a1dc3a8
to
0ca8680
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was quick. While doing the job I have two concerns:
- Seperation: so far dataclass was an independent module which does not know unitxt. It was done so to avoid circular imports and allow any unitxt module to use dataclass. I rather keep it that way if possible.
- Self-Contained Logic: Every class in unitxt should be self contained and understood from its definition. That way when someone read the code of class they can understand the way it works. Having the deprecation declared in the settings dis obey this principle.
If we don't find other way, this PR does answer the need for more informed field deprecation process.
My alternative suggestion is to create in the data class a new field type: DeprecatedField which will still be a viable field just produce a warning with a custom message given to the field constructor. That way both my concerns are addressed.
What do you think @dafnapension ?
…f code Signed-off-by: dafnapension <dafnashein@yahoo.com>
Signed-off-by: dafnapension <dafnashein@yahoo.com>
0ca8680
to
761746e
Compare
I think that you are right as always, @elronbandel , and I respectfully accept your comment, and will implement! |
Signed-off-by: dafnapension <dafnashein@yahoo.com>
Hi @elronbandel , please see if my implementation gets any close to the elegant solution you had in mind. |
Signed-off-by: dafnapension <dafnashein@yahoo.com>
Hi @elronbandel , please see my implementation per your metadata idea |
No description provided.