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

feat: deprecate FieldMeta collection params #417

Merged

Conversation

adhtruong
Copy link
Collaborator

Pull Request Checklist

  • New code has 100% test coverage
  • (If applicable) The prose documentation has been updated to reflect the changes introduced by this PR
  • (If applicable) The reference documentation has been updated to reflect the changes introduced by this PR
  • Pre-Commit Checks were ran and passed
  • Tests were ran and passed

Description

Close Issue(s)

@adhtruong adhtruong requested review from a team as code owners October 19, 2023 10:26
Copy link
Member

@guacs guacs left a comment

Choose a reason for hiding this comment

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

Thanks for the quick work! It looks great :)

@guacs guacs enabled auto-merge (squash) October 19, 2023 10:56
@adhtruong
Copy link
Collaborator Author

Something to note that this doesn't work for positional arg usage. This feels like reasonable limitation but happy to move warning usage into the methods themselves to be able to detect this.

@guacs
Copy link
Member

guacs commented Oct 19, 2023

Something to note that this doesn't work for positional arg usage. This feels like reasonable limitation but happy to move warning usage into the methods themselves to be able to detect this.

By this do you mean a check to see whether the value is set to a non-default value in those methods and then if so, raise the deprecation warning?

@adhtruong
Copy link
Collaborator Author

Something to note that this doesn't work for positional arg usage. This feels like reasonable limitation but happy to move warning usage into the methods themselves to be able to detect this.

By this do you mean a check to see whether the value is set to a non-default value in those methods and then if so, raise the deprecation warning?

Exactly. Theoretically this could be done by a decorator but seems overly complex for this use case so in body would be my preference if want to flag positional usage.

@guacs
Copy link
Member

guacs commented Oct 19, 2023

Something to note that this doesn't work for positional arg usage. This feels like reasonable limitation but happy to move warning usage into the methods themselves to be able to detect this.

By this do you mean a check to see whether the value is set to a non-default value in those methods and then if so, raise the deprecation warning?

Exactly. Theoretically this could be done by a decorator but seems overly complex for this use case so in body would be my preference if want to flag positional usage.

Yeah, I agree, a decorator would be too much. I think this a good idea and I remember this being done in litestar as well though I don't remember exactly where.

@adhtruong
Copy link
Collaborator Author

Got it. I'll update so positional usage is flagged too

auto-merge was automatically disabled October 19, 2023 12:22

Head branch was pushed to by a user without write access

@guacs
Copy link
Member

guacs commented Oct 21, 2023

@adhtruong I just had that one doubt regarding the naming. Everything else looks great!

@guacs guacs enabled auto-merge (squash) October 21, 2023 09:35
@guacs guacs merged commit 0717951 into litestar-org:main Oct 21, 2023
19 checks passed
@github-actions
Copy link

Documentation preview will be available shortly at https://litestar-org.github.io/polyfactory-docs-preview/417

@adhtruong adhtruong deleted the feat-deprecate-fieldmeta-collection-params branch October 21, 2023 09:43
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.

Enhancement: Deprecate collection params in FieldMeta.from_type
2 participants