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

Use json.Number #474

Merged
merged 9 commits into from
May 1, 2024
Merged

Use json.Number #474

merged 9 commits into from
May 1, 2024

Conversation

mperham
Copy link
Collaborator

@mperham mperham commented Apr 23, 2024

As #395 notes, unmarshalling JSON in Go can reduce precision for untyped numeric elements (any or interface{}) because it defaults to using Float64 as the type. We can lose precision as there are some Int64 values which can't be precisely represented as a Float64.

This PR aims to do the following:

  1. Provide a means for the user to enable Decoder.UseNumber() so that Faktory will preserve numeric elements. This will be off by default in 1.x but can be enabled with a flag. This will be on by default in 2.x.
  2. Reduce the use of untyped JSON hashes. The main offender here is the client.Info() command which returns a nasty map[string]interface{} requiring loads of type assertions. Not ergonomic or idiomatic. Instead we provide a new client.CurrentState() API which returns a client.FaktoryState structure which is almost entirely typed.

The test suite runs green locally with UseNumber = true or false.

Related to #395

@mperham
Copy link
Collaborator Author

mperham commented May 1, 2024

I'm backing off this change. I think the better approach is to adjust the job definition in v2 to use generics so that jobs can define a typed struct for their args which can be safely marshalled by JSON. The larger issue is defining a migration path away from Args []interface{}.

@mperham mperham merged commit 428506c into main May 1, 2024
3 checks passed
@mperham mperham deleted the use_json_numbers branch May 1, 2024 20: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.

1 participant