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

Export Helper Function #84

Merged
merged 6 commits into from
Apr 18, 2024
Merged

Export Helper Function #84

merged 6 commits into from
Apr 18, 2024

Conversation

mfleader
Copy link
Member

@mfleader mfleader commented Apr 17, 2024

Changes introduced with this PR

Facilitate type casting schemas to ObjectSchema outside of the pluginsdk package.

  • export the function previously known as convertToObjectSchema
  • refactor ConvertToObjectSchema method as a package function

By contributing to this repository, I agree to the contribution guidelines.

@mfleader mfleader marked this pull request as ready for review April 17, 2024 19:15
@mfleader mfleader self-assigned this Apr 17, 2024
@mfleader mfleader requested a review from webbnh April 17, 2024 19:16
schema/object.go Outdated Show resolved Hide resolved
schema/object.go Outdated Show resolved Hide resolved
Copy link
Contributor

@jaredoconnell jaredoconnell left a comment

Choose a reason for hiding this comment

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

Looks good to me. We'll see if Webb or Dave have any feedback.

Copy link

@dbutenhof dbutenhof left a comment

Choose a reason for hiding this comment

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

I don't see a technical issue, but the function comment could use some cleanup.

schema/object.go Outdated Show resolved Hide resolved
schema/object.go Outdated Show resolved Hide resolved
@mfleader mfleader requested a review from dbutenhof April 18, 2024 15:56
Copy link

@dbutenhof dbutenhof 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 I shouldn't really penalize you for not improving a pre-existing bad comment (even though you moved it), so, 99% ... 😆 👍🏻

schema/object.go Outdated Show resolved Hide resolved
@mfleader mfleader merged commit 32a9490 into main Apr 18, 2024
3 checks passed
@mfleader mfleader deleted the toObjSchema branch April 18, 2024 18:41
Copy link
Contributor

@webbnh webbnh left a comment

Choose a reason for hiding this comment

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

Ah, well...I was too slow. 👟

Comment on lines +662 to +676
// Try plain object schema
objectSchemaType, ok := typeOrData.(*ObjectSchema)
if ok {
return objectSchemaType, true
}
// Next, try ref schema
refSchemaType, ok := typeOrData.(*RefSchema)
if ok {
return refSchemaType.GetObject(), true
}
// Next, try scope schema.
scopeSchemaType, ok := typeOrData.(*ScopeSchema)
if ok {
return scopeSchemaType.Objects()[scopeSchemaType.Root()], true
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like it should be a switch statement, no?

	switch v := typeOrData.(type) {
	case *ObjectSchema:
		return v, true
	case *RefSchema:
		return v.GetObject(), true
	case *ScopeSchema:
		return v.Objects()[scopeSchemaType.Root()], true
	}

Copy link
Contributor

Choose a reason for hiding this comment

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

It's worth trying. If it fails, lots of tests for ValidateCompatibility will fail.

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

4 participants