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

Support marshal with linebreak and indent and support ignore specific struct field #150

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

funte
Copy link

@funte funte commented Aug 30, 2024

  1. For human readable in debug, add some code to support Schema.MarshalJSON marshal json with linebreak and indent, the default json output is still compact;
var MarshalWithIndent = false
var MarshalPrefix = ""
var MarshalIndent = "\t"
  1. Add optional Reflector.Ignore method to ignore specific struct field, enable dynamic generate different schemas from one struct.
Ignore func(reflect.StructField) bool

Copy link
Contributor

@samlown samlown left a comment

Choose a reason for hiding this comment

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

Tests are essential here to understand what you're trying to achieve. Also, I'm not sure indentation should be handled in the MarshalJSON method.

Comment on lines +1092 to +1094
var MarshalWithIndent = false
var MarshalPrefix = ""
var MarshalIndent = "\t"
Copy link
Contributor

Choose a reason for hiding this comment

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

Global variables should be avoided. Wouldn't you want to do this with the regular json.MarshalIndent method directly?

@@ -122,6 +122,9 @@ type Reflector struct {
// switching to just allowing additional properties instead.
IgnoredTypes []any

// Ignore specific struct field, enable dynamic generate different schemas from on struct.
Ignore func(reflect.StructField) bool
Copy link
Contributor

Choose a reason for hiding this comment

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

Its not clear how this should be used... as a rule, I'd try to avoid using reflect types directly.

@samlown samlown added the needs tests Not enough tests for this to be accepted label Oct 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs tests Not enough tests for this to be accepted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants