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 decorators for functions #221

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

Conversation

kunaljubce
Copy link
Contributor

@kunaljubce kunaljubce commented Mar 29, 2024

Proposed changes

Took another stab at #140, as an extension to #144.

Types of changes

What types of changes does your code introduce to quinn?
Put an x in the boxes that apply

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Update (if none of the other choices apply)

@SemyonSinchenko
Copy link
Collaborator

Is it a breaking change, isn't it? If so, I would suggest pushing it into 1.0, not main. In the same moment, we need to mark these functions as deprecated in the main and make a new release. @MrPowers what do you think?

@kunaljubce
Copy link
Contributor Author

Is it a breaking change, isn't it? If so, I would suggest pushing it into 1.0, not main. In the same moment, we need to mark these functions as deprecated in the main and make a new release. @MrPowers what do you think?

Could be a potential breaking change, yes. Would like to discuss more on this. Let me move this 1.0.

@kunaljubce
Copy link
Contributor Author

I don't see a branch for 1.0. Did we create it? I remember seeing a label for 1.0, though.

@kunaljubce
Copy link
Contributor Author

@SemyonSinchenko Also we are not exactly deprecating this functionality but evolving it to serve a better purpose, if I may say so 😆 Would a DeprecationWarning be apt for this?

I would think a better way would be to publish this change under a change log or something.

@kunaljubce
Copy link
Contributor Author

kunaljubce commented Mar 29, 2024

Implementation details for validate_schema

So basically we are implementing a decorator factory here so that our function can be used both as a decorator as well as a callable function. In this implementation:

  • The validate_schema function acts as both a decorator factory and a decorator. It takes required_schema, ignore_nullable, and an optional _df argument.
  • If _df is None, it means the function is being used as a decorator factory, and it returns the decorator decorator.
  • If _df is not None, it means the function is being called directly with a DataFrame, and it applies the decorator to _df immediately.

When validate_schema is called directly with a DataFrame, the validation logic gets executed by wrapping the DataFrame in a lambda function and immediately calling the decorator.

@SemyonSinchenko
Copy link
Collaborator

Hmm, maybe it would be better to leave a current function as is and create a new one? I don't know. @MrPowers @jeffbrennan, what do you think, guys?

@jeffbrennan
Copy link
Collaborator

jeffbrennan commented Mar 29, 2024

I like the idea of having one function that uses the decorator if no dataframe is provided - we would need to keep the arguments consistent with the current validate_schema implementation though

@kunaljubce
Copy link
Contributor Author

I like the idea of having one function that uses the decorator if no dataframe is provided - we would need to keep the arguments consistent with the current validate_schema implementation though

@jeffbrennan The only difference in the arguments is the absence of _df since this will be fetched from the base function on which the decorator is applied. The other two arguments required_schema and ignore_nullable are consistent across both scenarios. Is this what you are talking about or did I miss your point?

@kunaljubce
Copy link
Contributor Author

kunaljubce commented Mar 29, 2024

Hmm, maybe it would be better to leave a current function as is and create a new one? I don't know. @MrPowers @jeffbrennan, what do you think, guys?

@SemyonSinchenko This was the case with #141. I agree with the discussion in there that two different functions achieving a similar objective, one being used as a callable and one being used as a decorator can be confusing to users. That, in fact, was my inspiration to take a fresh attempt at this problem.

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.

None yet

3 participants