search

corbym / gocrest

GoCrest - Hamcrest-like matchers for Go
BSD 3-Clause "New" or "Revised" License
102 stars 6 forks source link

more boilerplate for has.Length() and is.Nil() after upgrade 1.1.0 release #9

Closed nitram509 closed 1 year ago

nitram509 commented 1 year ago

Hi,

thank you for your continuous effort to maintain this great lib.

With the new 1.1.0 release, I read you implemented generics support. I have to admit, without looking into the release notes, I did accept the PR from dependabot and was surprised, my pet project did not compile anymore. May I share some feedback? Introducing generics is good, but this breaking change should have been mentioned in the Release Notes. Also, I don't like the extra boilerplate code, I have to write, just to make the compiler happy. Is there a chance of introducing generics, with keeping the existing test code intact? And maybe omit the boilerplate code?

before (it works with gocrest 1.0.x
// from type inference, settings is of type []PoePortSetting
settings, err := findPortSettingsInHtml(strings.NewReader(getPoePortConfigCgiHtml))

then.AssertThat(t, err, is.Nil())
then.AssertThat(t, settings, has.Length(4))
after (to make it work with gocrest 1.1.x
// from type inference, settings is of type []PoePortSetting
settings, err := findPortSettingsInHtml(strings.NewReader(getPoePortConfigCgiHtml))

then.AssertThat(t, err, is.Nil[error]())
then.AssertThat(t, settings, has.Length[PoePortSetting, []PoePortSetting](4))

PS: the source, I'm talking about https://github.com/nitram509/ntgrrc/blob/main/poe_settings_test.go

Looking forward to hear your thoughts.

corbym commented 1 year ago

My apologies for breaking your project, my version change should probably have been a 2.0.x version in hindsight. It was a bigger change than I anticipated, but I believed that the changes would be mostly compatible with minor tweaks.

As mentioned in the readme, several things have changed since 1.0.x with generics, and whilst most of the API should remain similar with minimal changes, things like ValueContaining, is.Nil and has.Length needed more attention . The boiler plate required for is.Nil and has.Length, ironically, is to keep a similar API to 1.0.x. ValueContaining was replaced because I didn't think it was as well used.

The problem with Nil and Length is they can work on a number of entirely different types. The compiler, it seems, cannot determine the type itself (and I'm not entirely sure why). Without performing the same surgery on Nil and Length as was done on ValueContaining, I'm not sure if what you are requesting is possible.

I will have a look and see what can be done, and if you have any suggestions I'd be very grateful to receive a PR.

Thanks again for your feedback 👍

corbym commented 1 year ago

It might be worth mentioning to that the old version, without generics, is still supported for now, and is available at version v1.0.10

Thanks again for your support

corbym commented 1 year ago

@nitram509 please have a look at the PR above, and let me know your thoughts?

nitram509 commented 1 year ago

Hi, apologies for my late reply, I was off for some days. I looked at your above attempt and would agree, it looks better == simply from the point of writing fewer compiler hints. And thanks for your explanation about the reasoning ... maybe there's hope the next Go release will improve on the type inference.

Regards Martin