Skip to content
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

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

Closed
nitram509 opened this issue Apr 3, 2023 · 4 comments · Fixed by #10
Closed

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

nitram509 opened this issue Apr 3, 2023 · 4 comments · Fixed by #10

Comments

@nitram509
Copy link

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
Copy link
Owner

corbym commented Apr 4, 2023

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
Copy link
Owner

corbym commented Apr 4, 2023

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
Copy link
Owner

corbym commented Apr 6, 2023

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

@nitram509
Copy link
Author

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

corbym pushed a commit that referenced this issue Sep 6, 2024
# Conflicts:
#	is/nil.go
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 a pull request may close this issue.

2 participants