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 warning when set "alias" in low level annontated field #9170

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

Conversation

nix010
Copy link

@nix010 nix010 commented Apr 5, 2024

Change Summary

Add warning message when user setup annotated field with alias kwarg at low level. #7828.

Related issue number

Checklist

  • The pull request title is a good summary of the changes - it will be used in the changelog
  • Unit tests for the changes exist
  • Tests pass on CI
  • Documentation reflects the changes where applicable
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

Selected Reviewer: @adriangb

Copy link

codspeed-hq bot commented Apr 5, 2024

CodSpeed Performance Report

Merging #9170 will not alter performance

Comparing nix010:7828-add-warning-on-low-level-alias-in-annotated-field (fc394c5) with main (7501231)

Summary

✅ 13 untouched benchmarks

@nix010 nix010 changed the title Add warning in low level annontated field Add warning when set "alias" in low level annontated field Apr 5, 2024
@nix010
Copy link
Author

nix010 commented Apr 5, 2024

please review.

@sydney-runkle sydney-runkle added the relnotes-change Used for changes to existing functionality which don't have a better categorization. label Apr 14, 2024
Comment on lines +229 to +239
if hasattr(ann_type, '__args__'):
for anno_arg in ann_type.__args__:
if _typing_extra.is_annotated(anno_arg):
for anno_type_arg in _typing_extra.get_args(anno_arg):
if isinstance(anno_type_arg, FieldInfo) and anno_type_arg.alias is not None:
warnings.warn(
f'Field "{ann_name}" has `alias` should be set at higher level to have affect',
UserWarning,
)
break
break
Copy link
Contributor

@dmontagu dmontagu Apr 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if hasattr(ann_type, '__args__'):
for anno_arg in ann_type.__args__:
if _typing_extra.is_annotated(anno_arg):
for anno_type_arg in _typing_extra.get_args(anno_arg):
if isinstance(anno_type_arg, FieldInfo) and anno_type_arg.alias is not None:
warnings.warn(
f'Field "{ann_name}" has `alias` should be set at higher level to have affect',
UserWarning,
)
break
break
for anno_arg in getattr(ann_type, '__args__', ()):
if not _typing_extra.is_annotated(anno_arg):
continue
for anno_type_arg in _typing_extra.get_args(anno_arg):
if isinstance(anno_type_arg, FieldInfo) and anno_type_arg.alias is not None:
warnings.warn(
f'Field "{ann_name}" has `alias` should be set at higher level to have affect',
UserWarning,
)
break

Note: I removed the second break, and it seems to me like including it is problematic — if there are multiple annotated args it looks to me like it won't hit them.

I would prefer if we broke this out into a separate function (private in this module) to get this check moved out-of-line instead of making this method bigger and more complicated than it already is. (In that case, the break can just become return). And if it makes sense to change the code in a different way than suggested above after moving it into its own function, that's fine.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dmontagu Thank for the suggestions.
As my understand and please correct me if i'm wrong, warning on the first annotated args that has alias set or on all annotated args that have it is the same things !?.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nix010,

Thanks for your work on this.

I think we want to warn for each argument where it's a problem, which is what @dmontagu is pointing out.

I agree with his suggestions above! Feel free to ping me when you've made the changes, and I can do a second review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review relnotes-change Used for changes to existing functionality which don't have a better categorization.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants