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

return attrs for newly created object in rewrite #823

Merged
merged 3 commits into from Jun 9, 2022

Conversation

jess-sheneberger
Copy link
Contributor

When using Copier.Run to copy a file the resulting attributes object had 0 for generation. This change seems to populate the generation field with the value from the created object, but I'm not certain it's actually the correct fix--maybe the other attributes should be from newObject?

Please let me know if you need more information about the issue or the change. The tests appear to be passing.

Copy link
Owner

@fsouza fsouza left a comment

Choose a reason for hiding this comment

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

Hmm that should work yeah. Any chance we can add a test?

Thanks for contributing!

@jess-sheneberger
Copy link
Contributor Author

I just pushed a change to the existing rewrite tests to ensure that generation is not zero. I'm not sure if it's possible to predict in advance the generation. Maybe the correct test is to try fetching the version of the file with the returned generation? Let me know what you think.

I verified that the updated tests fail when I revert my change.

@fsouza
Copy link
Owner

fsouza commented Jun 8, 2022

I just pushed a change to the existing rewrite tests to ensure that generation is not zero. I'm not sure if it's possible to predict in advance the generation. Maybe the correct test is to try fetching the version of the file with the returned generation? Let me know what you think.

This looks good, can you just fix the golangci-lint failure? Thanks for contributing!

@fsouza
Copy link
Owner

fsouza commented Jun 9, 2022

Thank you!

@fsouza fsouza merged commit c5eaf3b into fsouza:main Jun 9, 2022
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

2 participants