rgbkrk / libvirt-go

[DEPRECATED] Go bindings for libvirt
https://github.com/libvirt/libvirt-go
MIT License
166 stars 50 forks source link

Strings & types to constants #74

Closed thegrumpylion closed 8 years ago

thegrumpylion commented 8 years ago

Hey guys!

I'd like to add types to constants and String() methods to aid with debugging & logging. But there are some issues.

1) When types are added it would be awesome to use stringer. But in order to use it the CONST = C.CONST CONT_ELSE = C.CONST_ELSE ... syntax must be change and iota should be use. I haven't found a way around this from my tests. This creates another problem. The constants need to be manually synced with the c header files for the order. A way to be sure that each constant corresponds correctly to it's c counterpart is to write tests for each constants CONST1 VirType = iota CONST2 ... TestVirTypeConstant() { c := CONST1 if c != C.CONST1 fail }

2) Even if we have these stuff, the api should change to return proper types instead of ints to be fully useful. This will break all the code that depends on this library.

Btw, is there any reason why this is not implemented already that i'm missing?

I'd love to here your thoughts on this.

Cheers!

P.S. Also, sort them by name :)

yalue commented 8 years ago

Somebody seems to have proposed this in issue #9, which was, for better or worse, ultimately rejected exactly because of what you stated: it "will break all the code that depends on this library."

I agree that adding a type that implements the String() function could be useful for debugging, but as for the use of iota, I would strongly suggest against that--you wouldn't want to get into a situation where an update to the libvirt headers would break the entire library in an unpredictable fashion. Plus, not all constants in libvirt may be simply increasing or decreasing in a way that iota can capture. Finally, several large go projects which wrap C libraries (such as go-sdl2) simply reference their constants to C.CONST_WHATEVER in the same way that this one currently does. It is certainly verbose, but it seems to be common practice at this point in cgo wrappers.

thegrumpylion commented 8 years ago

@yalue thanks for your input! I'll close this because it's not critical rather a convenience issue.