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(amber): add property matcher support #245

Merged
merged 32 commits into from
Jun 9, 2020
Merged

feat(amber): add property matcher support #245

merged 32 commits into from
Jun 9, 2020

Conversation

iamogbz
Copy link
Collaborator

@iamogbz iamogbz commented May 31, 2020

Description

Support property matchers

Related Issues

Checklist

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

Additional Comments

@codecov
Copy link

codecov bot commented May 31, 2020

Codecov Report

Merging #245 into master will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #245   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           16        18    +2     
  Lines          880       939   +59     
=========================================
+ Hits           880       939   +59     

@iamogbz iamogbz requested a review from noahnu May 31, 2020 06:43
@iamogbz iamogbz marked this pull request as ready for review May 31, 2020 21:09
src/syrupy/types.py Outdated Show resolved Hide resolved
@iamogbz iamogbz requested a review from noahnu June 3, 2020 12:07
Copy link
Collaborator

@noahnu noahnu left a comment

Choose a reason for hiding this comment

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

Generally LGTM. Can you update the README.md? We don't have this new behaviour documented anywhere

@iamogbz
Copy link
Collaborator Author

iamogbz commented Jun 3, 2020

Generally LGTM. Can you update the README.md? We don't have this new behaviour documented anywhere

@noahnu will do. Do you have any thoughts on the potential breaking nature of the change.

@noahnu
Copy link
Collaborator

noahnu commented Jun 3, 2020

Since the serializer is a public API, yes I believe it's a breaking change. How do we prevent this in the future? A note to use **kwargs? What do other python projects do?

@iamogbz iamogbz requested a review from noahnu June 3, 2020 19:55
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@noahnu
Copy link
Collaborator

noahnu commented Jun 5, 2020

If you recall the Django case that came up, where we don't want to load an attribute, is there a way to match before the attribute is accessed? Some way to check type without triggering a property get.

@iamogbz iamogbz requested a review from noahnu June 7, 2020 01:09
DataSerializer.write_file(snapshot_fossil_to_update)


__all__ = ["AmberSnapshotExtension", "DataSerializer"]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@noahnu keeping this for backwards compat, but I think the DataSerializer should be an internal interface

noahnu
noahnu previously approved these changes Jun 9, 2020
Copy link
Collaborator

@noahnu noahnu left a comment

Choose a reason for hiding this comment

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

Minor doc comments. Feel free to merge after fixing docs.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
| Argument | Description |
| --------- | ---------------------------------------------------------------------------------------------------------------------------------- |
| `mapping` | Dict of path string to tuples of class types |
| `types` | Tuple of class types used if none of the path strings from the mapping are matched |
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it only work on class types? What about primitive types?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@iamogbz iamogbz merged commit 83ded3c into master Jun 9, 2020
@iamogbz iamogbz deleted the property-matcher branch June 9, 2020 12:18
syrupy-bot pushed a commit that referenced this pull request Jun 9, 2020
# [0.5.0](v0.4.4...v0.5.0) (2020-06-09)

### Features

* **amber:** add property matcher support ([#245](#245)) ([83ded3c](83ded3c))
@syrupy-bot
Copy link
Contributor

🎉 This PR is included in version 0.5.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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.

3 participants