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 x-internal #64

Merged
merged 6 commits into from
Dec 22, 2021
Merged

Add x-internal #64

merged 6 commits into from
Dec 22, 2021

Conversation

nikicc
Copy link
Contributor

@nikicc nikicc commented Dec 1, 2021

This PR adds the support for marking the endpoints as x-internal, which is a mechanism for hiding endpoints in the rendered docs.

Background

Some docs rendering tools (e.g. ReDoc and Elements) support the functionality of the endpoints to be hidden from the docs, if they are marked as x-internal. The changes here add the support for x-internal.

@nikicc nikicc marked this pull request as ready for review December 1, 2021 18:56
@nikicc
Copy link
Contributor Author

nikicc commented Dec 1, 2021

@wI2L wdyt?

@nikicc nikicc mentioned this pull request Dec 3, 2021
fizz.go Outdated
@@ -360,6 +360,13 @@ func XCodeSample(cs *openapi.XCodeSample) func(*openapi.OperationInfo) {
}
}

// XInternal marks the operation as internal or external.
func XInternal(isInternal bool) func(*openapi.OperationInfo) {
Copy link
Owner

@wI2L wI2L Dec 22, 2021

Choose a reason for hiding this comment

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

Thought we might not need an argument, since the default behavior is external (internal: false), so the option func could be used as simply as XInternal().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. I removed it now.

@wI2L
Copy link
Owner

wI2L commented Dec 22, 2021

I merged some old pending PR, you need to rebase from the master branch.

@@ -194,6 +194,7 @@ type Operation struct {
Deprecated bool `json:"deprecated,omitempty" yaml:"deprecated,omitempty"`
Servers []*Server `json:"servers,omitempty" yaml:"servers,omitempty"`
XCodeSamples []*XCodeSample `json:"x-codeSamples,omitempty" yaml:"x-codeSamples,omitempty"`
XInternal bool `json:"x-internal,omitempty" yaml:"x-internal,omitempty"`
Copy link
Owner

Choose a reason for hiding this comment

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

What does the spec say about it ? Is it preferable if its omitted when its false (external), or should it be present for clarity and/or compatibility with the rendering interfaces ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm. Can't find anything really 🙈 Maybe it's a bit unclear as this is an extension, so I'm not sure it's covered in the spec?

Given that this is an extension, I'm leaning towards not showing x-internal: false (which should be the current behaviour) to not "pollute" the spec with default values of extensions. WDYT?

Copy link
Owner

Choose a reason for hiding this comment

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

Agreed, lets keep the omitempty flag then.

Copy link
Contributor Author

@nikicc nikicc left a comment

Choose a reason for hiding this comment

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

@wI2L ready for another look 👀

fizz.go Outdated
@@ -360,6 +360,13 @@ func XCodeSample(cs *openapi.XCodeSample) func(*openapi.OperationInfo) {
}
}

// XInternal marks the operation as internal or external.
func XInternal(isInternal bool) func(*openapi.OperationInfo) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. I removed it now.

@@ -194,6 +194,7 @@ type Operation struct {
Deprecated bool `json:"deprecated,omitempty" yaml:"deprecated,omitempty"`
Servers []*Server `json:"servers,omitempty" yaml:"servers,omitempty"`
XCodeSamples []*XCodeSample `json:"x-codeSamples,omitempty" yaml:"x-codeSamples,omitempty"`
XInternal bool `json:"x-internal,omitempty" yaml:"x-internal,omitempty"`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm. Can't find anything really 🙈 Maybe it's a bit unclear as this is an extension, so I'm not sure it's covered in the spec?

Given that this is an extension, I'm leaning towards not showing x-internal: false (which should be the current behaviour) to not "pollute" the spec with default values of extensions. WDYT?

@codecov
Copy link

codecov bot commented Dec 22, 2021

Codecov Report

Merging #64 (075f606) into master (1ced8ff) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #64      +/-   ##
==========================================
+ Coverage   95.17%   95.18%   +0.01%     
==========================================
  Files           7        7              
  Lines         953      956       +3     
==========================================
+ Hits          907      910       +3     
  Misses         30       30              
  Partials       16       16              
Impacted Files Coverage Δ
openapi/spec.go 81.57% <ø> (ø)
fizz.go 98.68% <100.00%> (+0.01%) ⬆️
openapi/generator.go 94.26% <100.00%> (+<0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1ced8ff...075f606. Read the comment docs.

README.md Outdated
@@ -111,8 +111,8 @@ fizz.WithoutSecurity()
// Add a Code Sample to the operation.
fizz.XCodeSample(codeSample *XCodeSample)

// Mark the operation as internal or external.
fizz.XInternal(isInternal bool)
// Mark the operation as internal.
Copy link
Owner

Choose a reason for hiding this comment

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

Since this is an extension, it might be beneficial for the readers to indicate that this is interpreted by third-party tools, and the impact is only on visual rendering. No need for an essai :) just a quick note.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that can be helpful. Added a quick note now.

@wI2L wI2L merged commit 408c6fc into wI2L:master Dec 22, 2021
@nikicc nikicc deleted the x-internal branch December 22, 2021 16:55
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

2 participants