-
Notifications
You must be signed in to change notification settings - Fork 421
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
Refactor mock name generation #452
Refactor mock name generation #452
Conversation
return name | ||
} | ||
|
||
return string(unicode.ToUpper(r)) + name[n:] |
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.
by the way I made some quick benchmarks and this seems to be the best way to do this...it is much faster than string -> []rune -> string
conversions
pkg/generator_test.go
Outdated
mock := &Mockrequester_unexported{} | ||
// NewMockRequester_unexported creates a new instance of MockRequester_unexported. It also registers a cleanup function to assert the mocks expectations. | ||
func NewMockRequester_unexported(t testing.TB) *MockRequester_unexported { | ||
mock := &MockRequester_unexported{} |
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.
Unfortunately this is a breaking change so we'll need to revert this back to the old (incorrect) behavior :'(
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.
Yeah, I thought so, no problem. But please, keep that in mind for v3, I think this should be fixed.
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.
Totally agreed. I will keep this PR open and add it to the v3 list.
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.
Okey, so, should I add the breaking change back? :D I think it would be better to merge this now (it's non-BC), and I'll post another PR for v3...what do you think?
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 see yeah we can get this merged now since you reverted the breaking change. Thanks!
6e96703
to
749b2d6
Compare
Codecov Report
@@ Coverage Diff @@
## master #452 +/- ##
==========================================
- Coverage 70.76% 70.75% -0.02%
==========================================
Files 7 7
Lines 1293 1289 -4
==========================================
- Hits 915 912 -3
+ Misses 325 324 -1
Partials 53 53
Continue to review full report at Codecov.
|
Also fix name for unexported InPackage mocks (was Mocklowercase and nowis MockUppercase, as expected)
This is a response PR to #406 (comment)
I didn't find any similar usages across the code base, but maybe I just missed them? If so, please, point me to them and I'll have another look at it :)
Please note that this changes how private mocks are generated when they are called with
--exported
. If this is an issue (it's BC), I can revert it/change it, but I think this is a bug fix and not a "change" per say... depends :) Your call.