-
Notifications
You must be signed in to change notification settings - Fork 17
[ECO-4688] Fix tests #391
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
[ECO-4688] Fix tests #391
Conversation
umair-ably
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
couple formatting issues - happy for these to be fixed in your upcoming PR if that's easier
lawrence-forooghian
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left some comments (and agree with Umair re all the unnecessary formatting changes).
I’d also like to mention that I found this PR difficult to review — in my opinion, unnecessarily so. There weren't any instructions on how to review it, so I tried reviewing it commit by commit. Unfortunately, there are many commits which turn out to be reverted by later commits, meaning that reviewing those commits turned out to be a waste of my time. Furthermore, the motivation for some of the changes is not very obvious, and isn't explained in the commit message, meaning that I had to spend time trying to understand what your intention was. I worry that if such a small PR was quite hard to review, then some of your upcoming larger protocol v2 ones will be very hard to review. Please could I ask that you spend time on those PRs to consider the experience for somebody trying to review them?
|
Thanks for approval 👍 |
Uh oh!
There was an error while loading. Please reload this page.