-
-
Notifications
You must be signed in to change notification settings - Fork 294
Add option 'create_error_stubs' (default is 'true') #508
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
Conversation
Proposal for issue ThrowTheSwitch#507
Hi @rstahn -- thanks for another well-thought-out patch. A couple of things: |
I am willing to extend documentation and test for sure -- see my comment in issue #507. I just wanted to ask for a first feedback before investing additional work. I will work on an improved PR this week. Next time maybe I should add the comment right here in the PR?! I am still trying to get more familiar with the common practice for OSS GitHub projects... |
Hi @rstahn -- I apologize for missing your comment on the related issue. That was a fine place to put that information. I don't know what the "common" practice for Github projects is... I honestly suspect I don't use half the tools properly... Personally, if there is a PR, I tend to push most of the conversation there, because it's focused on the solution? But that's not exactly consistent. ;) Nice work with the test fixes :) |
Hi @mvandervoord - thanks for the feedback. I fully agree with you, that discussion on implementation details fit well here in a PR. I will adopt that as rule of thumb going forward. 👍 Unfortunately I did not find the time to finish work on the PR this week. I will try again next week. 🤞 |
No worries. If I find the time, I'll finish it myself. Either way, thanks for the work! I really appreciate it! |
Change-Id: Ica84be72a229a4fd90e3d5ce0b3449722699f637
Change-Id: I33728023c1397e2bdbe62304638f578482ec8f58
Hi @mvandervoord, I've extended the documentation for To round things off I would like to extend the test coverage to include tests with |
That's a perfectly fine way to go about it, @rstahn . You don't need to include ALL the tests... just include enough tests that it proves the change you made and remove the rest from the new file file. :) |
Good point, @mvandervoord. I have now created a new set of tests IMHO the PR is now ready for review and merging. Should I combine all changes in my branch into a single commit? |
Remove trigger for branch rstahn-patch-1 for final PR
You can combine them into a single commit if you like. It's not really part of our process. I'll merge it either way. ;) |
My first quick attempt to squash my commits was not successful - so maybe I just leave it the way it is. 😄 |
Thanks for all your time and attention to this issue, @rstahn . The project has improved because of your efforts! |
Proposal for issue #507