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(serializer): add support for FunctionType serialization #823

Merged
merged 4 commits into from
Oct 24, 2023

Conversation

ManiacDC
Copy link
Contributor

@ManiacDC ManiacDC commented Oct 19, 2023

  • add FunctionType serialization in amber and json extensions
  • add tests

Description

add FunctionType serialization in amber and json extensions

Related Issues

Checklist

  • This PR has sufficient documentation.
  • This PR has sufficient test coverage.
  • This PR title satisfies semantic convention.

Additional Comments

I used inspect as I didn't want to re-invent the wheel. Is this satisfactory?

Also of minor concern - this changes the format of existing FunctionType serialization. I don't think this realistically causes a problem as the prior serialization would change every time it ran anyway.

* add FunctionType serialization in amber and json extensions
* add tests
@ManiacDC
Copy link
Contributor Author

@noahnu any idea why the Benchmark workflow is failing? Doesn't seem to be an issue with the PR changes.

@noahnu
Copy link
Collaborator

noahnu commented Oct 20, 2023

You can ignore the benchmark. It always fails for external contributors. There are credentials that don't get exposed.

I'll try to review this pull request soon, thank you for the contribution!

Copy link
Collaborator

@iamogbz iamogbz left a comment

Choose a reason for hiding this comment

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

LGTM

if isinstance(data, FunctionType):
return (
f"<{FunctionType.__name__} "
f"'{data.__qualname__}{str(inspect.signature(data))}'>"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Serialization nitpick

Suggested change
f"'{data.__qualname__}{str(inspect.signature(data))}'>"
f"{data.__qualname__}{str(inspect.signature(data))}>"

Result

"<function function_to_test(var1, var2='test_val', var3: str = 'test_val2', *, kwvar1, kwvar2='some_val') -> str>"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm OK with this, but this would be inconsistent with class, which surrounds in single quotes. However, I think because of the single quotes in the inspect signature, I'm going to go ahead with this change.

* remove snapshot invocations
* remove single quotes around json serialization due to single quotes in inspect signature.
@ManiacDC
Copy link
Contributor Author

Thanks for the review! I've updated with the suggested changes.

@noahnu
Copy link
Collaborator

noahnu commented Oct 24, 2023

Snapshot needs to be updated.

@ManiacDC
Copy link
Contributor Author

Snapshot needs to be updated.

Yeah I had left some testing code in the amber file by accident, fixed. Thanks!

@ManiacDC
Copy link
Contributor Author

Caught the formatting issue, but not fast enough. Sorry about that, used to format on save and I haven't setup vscode for this repo yet.

@noahnu noahnu merged commit f3a454a into syrupy-project:main Oct 24, 2023
13 of 14 checks passed
@noahnu
Copy link
Collaborator

noahnu commented Oct 24, 2023

@all-contributors add @ManiacDC for code

@allcontributors
Copy link
Contributor

@noahnu

I've put up a pull request to add @ManiacDC! 🎉

tophat-opensource-bot pushed a commit that referenced this pull request Oct 24, 2023
# [4.6.0](v4.5.0...v4.6.0) (2023-10-24)

### Features

* **serializer:** add support for FunctionType serialization ([#823](#823)) ([f3a454a](f3a454a))
@tophat-opensource-bot
Copy link
Contributor

🎉 This PR is included in version 4.6.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@ManiacDC
Copy link
Contributor Author

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

More generic serialization of function types
4 participants