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

feat: add initial code #2

Closed
wants to merge 1 commit into from
Closed

feat: add initial code #2

wants to merge 1 commit into from

Conversation

ccoVeille
Copy link
Owner

the test coverage is limited for now

I will need to consider to generate the tests.

Using generics in the code has huge consequences on unit tests

@ccoVeille
Copy link
Owner Author

@ldemailly could you please review ?

@ccoVeille ccoVeille force-pushed the draft branch 3 times, most recently from 99cd50f to b9308ff Compare September 5, 2024 21:10
the test coverage is limited for now

I will need to consider to generate the tests.

Using generics in the code has huge consequences on unit tests
@@ -0,0 +1,5 @@
module github.com/ccoveille/go-safecast

go 1.23.0
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1.23.1 is out

(and personally I would put just 1.23 so you don't have to keep changing it)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or more importantly... you don't want to force users to use 1.23, I wouldn't yet for anything prod

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's right, 1.22 is enough

I simply ran go mod init that set it up like this

Apparently, I'm still go 1.23.0 locally

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 328cfff

@ldemailly
Copy link
Collaborator

I think I have a much simpler implementation

package safecast

import (
	"errors"
)

type Integer interface {
	~int | ~uint | ~int8 | ~uint8 | ~int16 | ~uint16 | ~int32 | ~uint32 | ~int64 | ~uint64
}

var ErrOutOfRange = errors.New("out of range")

func Convert[Tout Integer, Tin Integer](orig Tin) (converted Tout, err error) {
	converted = Tout(orig)
	if Tin(converted) != orig {
		err = ErrOutOfRange
	}
	return
}

done, for all types!

@ldemailly
Copy link
Collaborator

sample test/demo:

func TestConvert(t *testing.T) {
	var inp uint32 = 42
	out, err := safecast.Convert[int8](inp)
	t.Logf("Out is %T: %v", out, out)
	if err != nil {
		t.Errorf("unexpected error: %v", err)
	}
	if out != 42 {
		t.Errorf("unexpected value: %v", out)
	}
	inp = 129
	_, err = safecast.Convert[int8](inp)
	t.Logf("Got err: %v", err)
	if err == nil {
		t.Errorf("expected error")
	}
}

outputs

$ go test -v .
=== RUN   TestConvert
    safecast_test.go:12: Out is int8: 42
    safecast_test.go:21: Got err: out of range
--- PASS: TestConvert (0.00s)
PASS

@ccoVeille
Copy link
Owner Author

unfortunately your implementation is not safe about the modulo that int conversion implies

int8(0) == int8(-256) == int8(256) == int8(512) ...

https://go.dev/play/p/0g3MBAC36we

I will iterate. Thanks for your inspiring example

@ccoVeille
Copy link
Owner Author

Closed in favor of #9

@ccoVeille ccoVeille closed this Sep 6, 2024
@ccoVeille ccoVeille deleted the draft branch September 6, 2024 19:55
@ccoVeille
Copy link
Owner Author

package safecast

import (
	"errors"
)

type Integer interface {
	~int | ~uint | ~int8 | ~uint8 | ~int16 | ~uint16 | ~int32 | ~uint32 | ~int64 | ~uint64
}

var ErrOutOfRange = errors.New("out of range")

func Convert[Tout Integer, Tin Integer](orig Tin) (converted Tout, err error) {
	converted = Tout(orig)
	if Tin(converted) != orig {
		err = ErrOutOfRange
	}
	return
}

done, for all types!

Unfortunately this implementation has some caveats

https://go.dev/play/p/irxvorEcy65

@ldemailly
Copy link
Collaborator

In case someone does archeology here, thanks for finding that bug, the fixed version is
https://github.com/ldemailly/go-scratch/blob/main/safecast/safecast.go

(added the 2 tests too)

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