search

ElektraInitiative / go-elektra-archive

Go bindings for Elektra
https://libelektra.org
BSD 3-Clause "New" or "Revised" License
5 stars 4 forks source link

`README.md` uses undefined method in sample code #19

Closed tmakar closed 1 year ago

tmakar commented 1 year ago

The README.md gives a sample code illustrating how elektra could be used. One of the used methods is Value which is expected to return the value of a Key. The Key does not provide such method and I have not seen any other method to fetch the value of a key. We should introduce a Value method on the Key type.

tmakar commented 1 year ago

I have now found that the method String is doing the job to return the value of a key as string. IMO the name is a bit misleading as I would expect String method on a Key to return the key name as string. I would suggest renaming it to Value, what do you think @kodebach @markus2330 ?

tmakar commented 1 year ago

I have also noticed that the sample provided in the README.md sends a SIGSEGV and crashes. It seems that there is something wrong with the NewKey method.

tmakar commented 1 year ago

The problem was that the README.md sample provided, states kdb.NewKey("user") which will result in an error as it can not create the key. This then leads to a SIGSEGV because no error handling was provided in the example and was not exiting before accessing a nil pointer.

kodebach commented 1 year ago
  1. The function is called String because it wraps the C function keyString. Which is called keyString, because it returns the keyvalue if it is a string value (and "(null)", "" or "(binary)" otherwise). I would keep this as-is for now. We are planning the change the C API see ElektraInitiative/libelektra#4853. Once we have agreed on a new C API (hopefully soon), we can update the Go to match that.
  2. AFAIK the Go binding hasn't been maintained for a while. It is very possible that some things are broken. It's especially likely that examples are out-of-date. As you noticed, the README.md contains an old example. kdb.NewKey("user") (or in C keyNew ("user")) has been wrong for a while now. It should be kdb.NewKey("user:/").
  3. I don't know what @markus2330 thinks, but IMO the best first step for working on this binding would be to reintegrate it into the main ElektraInitiative/libelektra repository. Since Go is very opinionated when it comes to modules and repository structure, it might be a bit difficult to do. But I think with replace in go.mod it should be possible to move the binding back into the main repo and have every thing run neatly in a single CI.
tmakar commented 1 year ago
  • The function is called String because it wraps the C function keyString. Which is called keyString, because it returns the keyvalue if it is a string value (and "(null)", "" or "(binary)" otherwise). I would keep this as-is for now. We are planning the change the C API see Proposals for new API libelektra#4853. Once we have agreed on a new C API (hopefully soon), we can update the Go to match that.
  • AFAIK the Go binding hasn't been maintained for a while. It is very possible that some things are broken. It's especially likely that examples are out-of-date. As you noticed, the README.md contains an old example. kdb.NewKey("user") (or in C keyNew ("user")) has been wrong for a while now. It should be kdb.NewKey("user:/").

Ok, I see. Just wanted to point it out cause I was a bit confused when I saw it and at first glance didn't know what it was doing. But I will then revert the renaming. The PR should now contains a sample with error handling and a fixed version and now does not crash.

I primarily started working in this repo because of the use case for globbing (spec plugin) I'm trying to write in Go.

markus2330 commented 1 year ago

I don't know what @markus2330 thinks, but IMO the best first step for working on this binding would be to reintegrate it into the main ElektraInitiative/libelektra repository

Yes, I would very much appreciate if it can be integrated in the main repository. This would solve several problems.