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

Chore: Move jsonitere outside of the internal package #837

Merged
merged 4 commits into from
Dec 15, 2023

Conversation

itsmylife
Copy link
Contributor

@itsmylife itsmylife commented Dec 13, 2023

What this PR does / why we need it:

With the current state we cannot use jsonitere package in grafana/grafana or in any project.
Please advise otherwise if you think the root is not a good place for that package.

image

@itsmylife itsmylife requested a review from kylebrandt December 13, 2023 15:31
@itsmylife itsmylife requested review from a team as code owners December 13, 2023 15:31
@itsmylife itsmylife requested review from wbrowne, andresmgot and xnyo and removed request for a team December 13, 2023 15:31
@marefr
Copy link
Contributor

marefr commented Dec 13, 2023

In the root doesn't feel optimal. Since data/frame_json.go is the only one using it maybe data/util/jsonitere or data/jsonitere. Depending on how/where you want to use it, maybe backend/utils/jsonitere or backend/jsonitere (but this one feels a bit misplaced)

Nit. Personally I find it hard to read the package name jsonitere and as long it's been internal doesn't matter that much, but making it public now I think we should take the opportunity and change it if there's consensus.

@itsmylife
Copy link
Contributor Author

We are gonna use it as a wrapper for "github.com/json-iterator/go" package. And then we use it everywhere we use github.com/json-iterator/go package. I agree that root is not an ideal place for this. I think backend/utils is much more appropriate.

Regarding the naming, do you have any suggestions? jsonwrapper maybe?

cc: @kylebrandt

@andresmgot
Copy link
Contributor

The library suggest using jsoniter as the package name so we could use the same: https://pkg.go.dev/github.com/json-iterator/[email protected]#readme-usage

I would go with data/utils for the package but backend/utils works too.

Apart from that, I'd suggest to add some basic test coverage, any package exposed should have that.

@itsmylife
Copy link
Contributor Author

I'll make the necessary changes. Testing makes sense.
@kylebrandt I need your input for this thread as well.

@kylebrandt
Copy link
Contributor

I like backend utils, since this is for parsing json, it doesn't have to be used with the data layer (even though that is practically when we would use it generally).

@itsmylife
Copy link
Contributor Author

@marefr @andresmgot I made the updates. Please check them. I am not sure what else I can test since this is just a wrapper.

Copy link
Contributor

@andresmgot andresmgot left a comment

Choose a reason for hiding this comment

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

I guess this is good enough to go, I have a couple of suggestions but not feeling strongly about those

@@ -17,7 +17,7 @@ import (
jsoniter "github.com/json-iterator/go"
"github.com/mattetti/filebuffer"

"github.com/grafana/grafana-plugin-sdk-go/internal/jsonitere"
jiter "github.com/grafana/grafana-plugin-sdk-go/data/utils/jsoniter"
Copy link
Contributor

Choose a reason for hiding this comment

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

sdkjsoniter ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this makes sense. I'll do the change.

Comment on lines +14 to +26
const (
InvalidValue = j.InvalidValue
StringValue = j.StringValue
NumberValue = j.NumberValue
NilValue = j.NilValue
BoolValue = j.BoolValue
ArrayValue = j.ArrayValue
ObjectValue = j.ObjectValue
)

var (
ConfigDefault = j.ConfigDefault
)
Copy link
Contributor

Choose a reason for hiding this comment

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

since you are mapping some of the things exported in the original package, maybe we should go over the rest of things that frame_json.go is using so it doesn't use directly github.com/json-iterator/go?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried that. But for this PR it'll make things much more complicated. Maybe I'll do it in a separate PR.

@itsmylife itsmylife merged commit 0afe3c9 into main Dec 15, 2023
1 check passed
@itsmylife itsmylife deleted the ismail/move-jsonitere-from-internal-package branch December 15, 2023 19:46
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.

4 participants