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

struct-tag rule false-positive on json inline tag #520

Closed
finnan444 opened this issue May 20, 2021 · 14 comments · Fixed by #802
Closed

struct-tag rule false-positive on json inline tag #520

finnan444 opened this issue May 20, 2021 · 14 comments · Fixed by #802
Assignees

Comments

@finnan444
Copy link

Describe the bug
struct-tag produces warning for json inline tag struct-tag: unknown option 'inline' in JSON tag (revive)

To Reproduce
Steps to reproduce the behavior:

  1. add
type Beta struct {
    Alpha `json:",inline"`
}
  1. Run revive with struct-tag rule enabled
@chavacava
Copy link
Collaborator

Hi @finnan444 thanks for filling the issue.
In fact inline is not an official option of JSON struct tags (this 8 years old but still ongoing discussion also talks about it)

(Actually, the inline option works by lucky it might be possible that if you change yours inline by something it will still working)

@finnan444
Copy link
Author

finnan444 commented May 21, 2021

Hi @chavacava, sorry for inconvenience, this tag is widely used in my company (this practice came from kubernetes repo), so i thought inline is part of standard now.
I suppose issue can be closed

@chavacava
Copy link
Collaborator

Do not worry, we have been all trapped by this inline option :)

Thanks again for taking the time of filling the issue.

@s3rj1k
Copy link

s3rj1k commented Apr 13, 2022

@chavacava So why not add a configuration option to struct-tag linter?

@chavacava
Copy link
Collaborator

@s3rj1k What do you need to configure?

@s3rj1k
Copy link

s3rj1k commented Apr 13, 2022

I was thinking of an array with additionally allowed list of tags, to make that inline tag allowed per user-configuration
(or any other none standard tag)

@chavacava
Copy link
Collaborator

Could you please develop on the interest of using non-standard options in struct tags?
Why not just remove inline from JSON tags? (unknown options are ignored by encoding/json)

@s3rj1k
Copy link

s3rj1k commented Apr 13, 2022

@chavacava inline tag is needed for kubernetes CRD creation, please see link below
https://book.kubebuilder.io/reference/generating-crd.html#additional-printer-columns

@chavacava
Copy link
Collaborator

I do not know about CRD creation but I know that non standard tag options are ignored by encoding\json so its hard to me to imagine what is the interest of using inline option.
Did you try to replace json:",inline" by just json:"" when creating a CRD? It should be semantically equivalent as this code shows

I was thinking of an array with additionally allowed list of tags, to make that inline tag allowed per user-configuration

If we add a configuration, it should allow additional options but scoped to particular tags. For example, by allowing inline for JSON tags does not allow it for other tags (ASN, csv, base64, ...)

@s3rj1k
Copy link

s3rj1k commented Apr 14, 2022

@chavacava inline is used across all K8s code, for example here https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/garbagecollector/metaonly/types.go#L30

I would rather use it also like docs say.

If we add a configuration, it should allow additional options but scoped to particular tags. For example, by allowing inline for JSON tags does not allow it for other tags (ASN, csv, base64, ...)

Sure, so a list per tag or a map[string][]string

@chavacava
Copy link
Collaborator

inline is used across all K8s code

I know. I use Kubernetes repo as the main sandbox for testing new linting rules because: a) it is a huge repo and, b) it is not famous for its coding style quality ;)

@s3rj1k
Copy link

s3rj1k commented Apr 14, 2022

So, either we have some kind of a configurable option for struct-tag, to allow this K8s mess and have other struct tags checked or we go and disable struct-tag linter for specific places or for all code base.

Personally I would be happy with configuration option, that way linter will work on other structs inside project.

@s3rj1k
Copy link

s3rj1k commented Mar 10, 2023

@chavacava Bump, I am forced to use exclude rule for this and this is ugly.

chavacava added a commit that referenced this issue Mar 11, 2023
@chavacava chavacava self-assigned this Mar 11, 2023
@chavacava
Copy link
Collaborator

@s3rj1k I've made the PR #802 to let users tag structs with all the unofficial options they can imagine.

mgechev pushed a commit that referenced this issue Mar 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants