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

LazyAssertion not open for extension #284

Closed
DannyvdSluijs opened this issue Jul 24, 2019 · 9 comments
Closed

LazyAssertion not open for extension #284

DannyvdSluijs opened this issue Jul 24, 2019 · 9 comments

Comments

@DannyvdSluijs
Copy link

I wanted to extend the LazyAssertion in order to overload the that method. As I need to pass some additional details into the exception created by the assertion lib.

With all the properties on the LazyAssertion are private this doesn't allow for easy extending.

Would you be willing to accept a PR which would change the properties on the LazyAssertion from private too protected? (Like the Assert class already has, which can easily be extended)

@rquadling
Copy link
Contributor

Happy to accept a PR. Please make sure you provide an example in a test covering the extension as an example usage. Also please read https://github.com/beberlei/assert/blob/master/CONTRIBUTING.md.

Looking forward to seeing this!

@DannyvdSluijs
Copy link
Author

DannyvdSluijs commented Aug 9, 2019

PR #285 has been created for this. If you could, at your convenience, have a look and let me know what you think of it.

I've made sure that:

@DannyvdSluijs
Copy link
Author

@rquadling nudging you as a gentle reminder to get your feedback on my PR. Thanks in advance.

@rquadling
Copy link
Contributor

Hi. Sorry! Looking.

@rquadling
Copy link
Contributor

See the PR for my comments. Please rebase onto master for more uptodate pipeline checks and consistencies.

@DannyvdSluijs
Copy link
Author

Thanks for the review, I'll pick it up from here.

@rquadling
Copy link
Contributor

I've had to release a fix for some BC's. Can you rebase and fix please?

@DannyvdSluijs
Copy link
Author

@rquadling will do a rebase but still havent found the time to makes changes based your review.

@DannyvdSluijs
Copy link
Author

Leaving the PR for what it is. Ive no longer need the changes proposed in this PR. If somebody wants to take over feel free.

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

No branches or pull requests

2 participants