Skip to content

Conversation

rstahn
Copy link
Contributor

@rstahn rstahn commented Jun 2, 2025

Proposal for issue #507

@mvandervoord
Copy link
Member

Hi @rstahn -- thanks for another well-thought-out patch. A couple of things:
(1) Would you mind adding the new option to the documentation?
(2) If you'd like to fix the self-tests yourself, you can add the new feature to the create_sub call in the before section of the unit tests.

@rstahn
Copy link
Contributor Author

rstahn commented Jun 3, 2025

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...

@mvandervoord
Copy link
Member

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 :)

@rstahn
Copy link
Contributor Author

rstahn commented Jun 6, 2025

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. 🤞

@mvandervoord
Copy link
Member

No worries. If I find the time, I'll finish it myself. Either way, thanks for the work! I really appreciate it!

rstahn and others added 2 commits June 8, 2025 22:57
Change-Id: I33728023c1397e2bdbe62304638f578482ec8f58
@rstahn
Copy link
Contributor Author

rstahn commented Jun 12, 2025

Hi @mvandervoord, I've extended the documentation for create_error_stubs now (after some troubles setting up git locally).

To round things off I would like to extend the test coverage to include tests with create_error_stubs set to false: As a first experiment I copied the existing unit test cmock_generator_plugin_expect_a_test.rb to a new file cmock_generator_plugin_expect_c_test.rb where create_error_stubs is set to false. This works, but I am not sure whether that is the best way to test the option. When I continue on that route, I need to duplicate various other tests as well. Do you see a better way?

@mvandervoord
Copy link
Member

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. :)

@rstahn
Copy link
Contributor Author

rstahn commented Jun 13, 2025

Good point, @mvandervoord. I have now created a new set of tests cmock_generator_no_error_stubs_plugin_*_test for all plugins affected by the create_error_stubs options. Looks much better to me. 👍

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
@mvandervoord
Copy link
Member

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. ;)

@rstahn
Copy link
Contributor Author

rstahn commented Jun 13, 2025

My first quick attempt to squash my commits was not successful - so maybe I just leave it the way it is. 😄

@mvandervoord mvandervoord merged commit bf9a13a into ThrowTheSwitch:master Jul 9, 2025
4 checks passed
@mvandervoord
Copy link
Member

Thanks for all your time and attention to this issue, @rstahn . The project has improved because of your efforts!

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.

2 participants