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

Wrap reflect.Type.ConvertibleTo calls with Go/TinyGo implementations #522

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kenshaw
Copy link

@kenshaw kenshaw commented Nov 12, 2024

Enable package to be usable with TinyGo.

Changes calls to reflect.Type.ConvertibleTo in decode.go to Go/TinyGo specific implementations. Basic examples work, but (some) unit tests fail as conversions to interface{} do not currently work in TinyGo.

Otherwise, all other functionality I've tested now appears to work properly: highlighted error output, cmd/ycat, the README.md examples, etc.

As an example, both Go and TinyGo are now able to use this package vis-a-vis this example taken from README.md:

package main

import (
	"fmt"
	"strings"

	"github.com/goccy/go-yaml"
)

func main() {
	yml := `
store:
  book:
    - author: john
      price: 10
    - author: ken
      price: 12
  bicycle:
    color: red
    price: 19.95
`
	path, err := yaml.PathString("$.store.book[*].author")
	if err != nil {
		//...
	}
	var authors []string
	if err := path.Read(strings.NewReader(yml), &authors); err != nil {
		//...
	}
	fmt.Println(authors)
}

Output:

ken@ken-desktop:~/g$ go run main.go
[john ken]
ken@ken-desktop:~/g$ tinygo run main.go
[john ken]
ken@ken-desktop:~/g$

(previously the tinygo run would panic)

Before submitting your PR, please confirm the following.

  • Describe the purpose for which you created this PR.
  • Create test code that corresponds to the modification

@codecov-commenter
Copy link

codecov-commenter commented Nov 12, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 79.72%. Comparing base (ff5d41f) to head (e1eea91).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #522   +/-   ##
=======================================
  Coverage   79.71%   79.72%           
=======================================
  Files          18       19    +1     
  Lines        6374     6376    +2     
=======================================
+ Hits         5081     5083    +2     
  Misses        986      986           
  Partials      307      307           

@kenshaw
Copy link
Author

kenshaw commented Nov 12, 2024

Hi, humbly requesting that this be added, as I have some command-line utilities that I'm switching over to to building with tinygo, and would like to use this package for YAML parsing.

Thanks in advance, and really appreciate the work on this package! Especially now that it has zero dependencies!

@goccy
Copy link
Owner

goccy commented Nov 12, 2024

Thank you for your contribution.

Is there any public information regarding the lack of support for reflect.Type.ConvertibleTo in TinyGo ?
Additionally, if we include TinyGo as a build target, ensuring that it can be continuously built with TinyGo will be required, so we need to consider the maintenance cost.

@kenshaw
Copy link
Author

kenshaw commented Nov 12, 2024

Appreciate the quick response. I'm happy to contribute to the CI/CD build workflow if necessary, but I would not suggest making TinyGo as a major compatibility target and/or "supported".

For those using TinyGo, they are most likely already aware of TinyGo's limitations, and know they are working with subset of Go to begin with. As such, they likely would appreciate any functionality/support, and they will likely test their code code sufficiently enough.

The request for this to be merge is because it doesn't affect regular Go in any fashion, but at least enables using the package with TinyGo. It's worth noting that the API used in the actual conv_tinygo.go is Go's standard, public reflect API, it's actually possible to build/compile this path with regular Go, if one were so inclined (it just wouldn't work in all scenarios).

This PR is just a helpful workaround for those who are using TinyGo, as otherwise TinyGo's reflect package panics when calling the ConvertibleTo directly. Hopefully in some future release of TinyGo they will get ConvertibleTo (and the rest of the reflect package and standard library) working, and at that point this change would be revertible to just the standard API.

Enable package to be usable with [TinyGo](https://tinygo.org).

Changes calls to reflect.Type.ConvertibleTo in decode.go to Go/TinyGo
specific implementations. Basic examples work, but (some) unit tests
fail as conversions to `interface{}` do not currently work in TinyGo.

Otherwise, all other functionality I've tested now appears to work
properly: highlighted error output, `cmd/ycat`, the README.md examples,
etc.

As an example, both Go and TinyGo are now able to use this package
vis-a-vis this example taken from README.md:

```go
package main

import (
	"fmt"
	"strings"

	"github.com/goccy/go-yaml"
)

func main() {
	yml := `
store:
  book:
    - author: john
      price: 10
    - author: ken
      price: 12
  bicycle:
    color: red
    price: 19.95
`
	path, err := yaml.PathString("$.store.book[*].author")
	if err != nil {
		//...
	}
	var authors []string
	if err := path.Read(strings.NewReader(yml), &authors); err != nil {
		//...
	}
	fmt.Println(authors)
}
```

Output:

```sh
ken@ken-desktop:~/g$ go run main.go
[john ken]
ken@ken-desktop:~/g$ tinygo run main.go
[john ken]
ken@ken-desktop:~/g$
```

(previously the `tinygo run` would panic)
@kenshaw
Copy link
Author

kenshaw commented Nov 17, 2024

I've rebased the commit to the latest master. Would appreciate any feedback on what would be necessary to get this added to the source tree. Happy to do any work necessary to get this accepted. Thanks!

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.

3 participants