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

feat: add float_format flag #367

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

Conversation

rhnvrm
Copy link

@rhnvrm rhnvrm commented Mar 21, 2022

This commit adds a float_format flag to easyjson which allows
users to change the float format used in strconv. The default flag
used is g by easyjson which is kept as is to avoid a breaking change.

If a user wishes, they can now provide a flag such as:
bin/easyjson -float_format='f' ./tests/float_format.go

The g format converts long floats into
e-notation. Using a flag such as 'f' might be necessary for certain
use cases to maintain backward compatibility.

This commit adds a float_format flag to easyjson which allows
users to change the float format used in strconv. The default flag
used is `g` by easyjson.

If a user now provides a flag such as:
	bin/easyjson -float_format='f' ./tests/float_format.go

It will use that format. The g format converts long floats into the
e-notation. Using a flag such as 'f' might be necessary in certain
usecases to maintain backward compatibility.
@rhnvrm rhnvrm mentioned this pull request Mar 21, 2022
@rhnvrm
Copy link
Author

rhnvrm commented Mar 21, 2022

Hey @bulletmys, we internally use this patch and were wondering if it would be possible to upstream it? Since there was not much activity on the repo recently, thought to ping.

gen/encoder.go Outdated Show resolved Hide resolved
@@ -39,6 +39,7 @@ type Generator struct {
fieldNamer FieldNamer
simpleBytes bool
skipMemberNameUnescaping bool
floatFmt string
Copy link
Collaborator

Choose a reason for hiding this comment

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

why not byte?

Copy link
Author

Choose a reason for hiding this comment

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

I'd made it string to keep it consistent with the incoming flag which would be string. Also, in case we use byte and pass it as empty, the generator sets it as:

w := jwriter.Writer{FloatFmt: "0"}

I could try figuring this out further if byte seems more relevant as the type of floatFmt, but I guess it should be ok to keep it as string?

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can check what value is correct on code generation stage and pass default value if cmd line parameter not passed

Copy link
Author

@rhnvrm rhnvrm Apr 8, 2022

Choose a reason for hiding this comment

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

So currently, since the flag package does not have support for byte/rune flags, it is a bit difficult to do this. I guess it might be okay to keep it as is (?), since the internal method converts the string to byte and sets the default value as well.

@rhnvrm
Copy link
Author

rhnvrm commented Oct 11, 2022

Hi @GoWebProd

Just wanted to bump this up.

@rhnvrm
Copy link
Author

rhnvrm commented Aug 21, 2023

@GoWebProd @bulletmys

Apologies for bumping this. We have been using the fork in production since March 2022. Would love to get it upstreamed. If any suggestions/tweaks I can do to align it with the project do let me know, would be happy to work on it.

@knadh
Copy link

knadh commented Nov 17, 2023

+1 Would be great if this PR could be considered.

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