Skip to content

feat: add generate options #150

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

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

Conversation

moecasts
Copy link

make generate configurable

// UseProtoNames controls the casing of generated field names.
// If set to true, fields will use proto names (typically snake_case).
// If omitted or set to false, fields will use JSON names (typically camelCase).
use_proto_names: bool

// UseEnumNumbers emits enum values as numbers.
use_enum_numbers: bool

// The method names of service methods naming case.
// Only work when `UseEnumNumbers=true`
// opt:
// camelcase: convert name to lower camel case like `camelCase`
// pascalcase: convert name to pascalcase like `PascalCase`
// default is pascalcase
enum_field_naming: string

// Generate comments as multiline comments.
// multiline comments: /** ... */
// single line comments: // ...
use_multi_line_comment: bool

// force add `undefined` to message field.
// default true
force_message_field_undefinable: bool

// If set to true, body will be JSON.stringify before send
// default true
use_body_stringify: bool

// The method names of service methods naming case.
// opt:
// camelcase: convert name to lower camel case like `camelCase`
// pascalcase: convert name to pascalcase like `PascalCase`
// default is pascalcase
service_method_naming: 'camelcase' | 'pascalcase'

// If set to true, field int64 and uint64 will convert to string
force_long_as_string: bool

Inspirit from:

@moecasts moecasts requested a review from ericwenn as a code owner October 14, 2023 18:10
@moecasts moecasts force-pushed the feature/add-generate-options branch 4 times, most recently from 3728202 to 52ca111 Compare October 15, 2023 14:09
Copy link
Member

@ericwenn ericwenn left a comment

Choose a reason for hiding this comment

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

Thanks for this PR!
Overall I'm positive to this change, but left some comments for improvments and questions.

Comment on lines +86 to +89
// This is optional
type RequestOptions = {
useCache?: boolean;
}
Copy link
Member

Choose a reason for hiding this comment

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

Where does this come from?

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see it now, see comment on the use_body_string option.

Comment on lines +49 to +52
// Generate comments as multiline comments.
// multiline comments: /** ... */
// single line comments: // ...
use_multi_line_comment: bool
Copy link
Member

Choose a reason for hiding this comment

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

Why does the comment formatting need to be configurable?
I'd rather pick one of the formats (single/multi) and always use that, not configurable.

Copy link
Author

@moecasts moecasts Oct 15, 2023

Choose a reason for hiding this comment

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

because multiline comments is readable in ide tips dialog.
image

image

and make comment formatting be configurable is to aviod breaking change(Maybe it's just me worrying too much).

Comment on lines +58 to +60
// If set to true, body will be JSON.stringify before send
// default true
use_body_stringify: bool
Copy link
Member

Choose a reason for hiding this comment

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

While I'm all for having this option, I understand it as you are also using that as a way to pass options to the request handler?

I would rather see a new argument to the request handler - something like handlerOptions: T - than spreading it into the request body.

Copy link
Author

@moecasts moecasts Oct 15, 2023

Choose a reason for hiding this comment

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

noop, at this monent, JSON.stringify is a mandatory behavior.

f.P(t(3), "const body = JSON.stringify(request);")
default:
nullPath := nullPropagationPath(httprule.FieldPath{rule.Body}, input)
f.P(t(3), "const body = JSON.stringify(request?.", nullPath, " ?? {});")

add this option is remove it, and the default value is true to avoid breaking change.

if someone like to use JSON.stringify, this option can be enabled, or archive it in requestHandler.

Comment on lines +69 to +70
// If set to true, field int64 and uint64 will convert to string
force_long_as_string: bool
Copy link
Member

Choose a reason for hiding this comment

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

Should this just be called long_as_string instead? Not sure where the force comes from ;)

Copy link
Author

@moecasts moecasts Oct 15, 2023

Choose a reason for hiding this comment

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

This name is borrowed from: https://github.com/stephenh/ts-proto
image

I would like to borrow from the existing naming rather than define a new one.

Of course, if you really don't like it, I can change it.

@moecasts moecasts force-pushed the feature/add-generate-options branch from 52ca111 to 0184e35 Compare October 15, 2023 18:30
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.

2 participants