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

Add EnsureNoExportedKindName check #8

Merged
merged 7 commits into from
Apr 27, 2023
Merged

Add EnsureNoExportedKindName check #8

merged 7 commits into from
Apr 27, 2023

Conversation

undef1nd
Copy link
Contributor

@undef1nd undef1nd commented Mar 23, 2023

Issue: in some cases using Kind as a schema field name leads to generating Kind type in Go that shadows existing type in that same package in Grafana.

Related to https://github.com/grafana/schematization-and-as-code-project/issues/31

@undef1nd undef1nd changed the title Add EnsureNoExportedKindName check [WIP] Add EnsureNoExportedKindName check Mar 23, 2023
@sdboyer
Copy link
Contributor

sdboyer commented Mar 23, 2023

I tried to call it in LoadCoreKindDef but vk.Source() there returns nil, so not possible to use it there.

The vk there is the root node of the whole kind. Gotta walk down to the schema elements, first.

Though, looking now, i realize this is awkward because LoadCoreKindDef() doesn't actually call thema.BindLineage(), so you can't do something like this, where thema makes it easy to get at the cue.Value for each schema:

	for sch := lin.First(); sch != nil; sch = sch.Successor() {
		if err := validation.EnsureNoExportedKindName(sch.Underlying()); err == nil {
			return err
		}
	}

Hmmm. Maybe that means it's best done here, instead. That feels to me like it's too late...but for now it's probably the most convenient spot.

@undef1nd
Copy link
Contributor Author

undef1nd commented Mar 23, 2023

The vk there is the root node of the whole kind. Gotta walk down to the schema elements, first

That should be probably fine, as I search for the lineage node first here. And then search for the keyword in the lineage node elements.

Copy link
Contributor

@sdboyer sdboyer left a comment

Choose a reason for hiding this comment

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

This approach looks good to me 👍🏻

Next question is where we hook in to apply it!

@undef1nd undef1nd changed the title [WIP] Add EnsureNoExportedKindName check Add EnsureNoExportedKindName check Apr 13, 2023
@undef1nd
Copy link
Contributor Author

Next question is where we hook in to apply it!

As we discussed in the chat, put it here in Grafana https://github.com/grafana/grafana/pull/66516/files

@sdboyer
Copy link
Contributor

sdboyer commented Apr 14, 2023

i think this should be good to mark as ready so that we can get it merged and progress on the other PR

@undef1nd undef1nd marked this pull request as ready for review April 14, 2023 19:57
@undef1nd undef1nd requested a review from sdboyer April 14, 2023 19:57
Copy link

@joanlopez joanlopez left a comment

Choose a reason for hiding this comment

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

I really like this approach, very clean and reusable! Great! 🚀

PS: Thinking out loud -- Could we add a unit test? It may also help understanding how to use the function from the consumer perspective.

@sdboyer
Copy link
Contributor

sdboyer commented Apr 17, 2023

Thinking out loud -- Could we add a unit test?

IMO definitely worth doing. It'll necessarily also be tested on our corpus once we have one, but it deserves a smaller unit test too

Copy link
Contributor

@sdboyer sdboyer left a comment

Choose a reason for hiding this comment

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

LGTM, too!

@sdboyer sdboyer merged commit 0aae354 into main Apr 27, 2023
@sdboyer sdboyer deleted the kind-name-check branch April 27, 2023 16:52
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.

None yet

3 participants