Skip to content

Conversation

@Eomm
Copy link
Member

@Eomm Eomm commented Jan 6, 2021

Fix #277

Note that to implement a rounding input option the most straightforward way is to generate the output function calling a global named function.

  let customAsInteger = $asInteger.toString()
  if (options.rounding) {
    customAsInteger = customAsInteger.replace('parseInt', options.rounding)
  }

In this case the input would be the ugly (note the Math.round as 'string`):

const debugCompiled = fastJson(schema, { debugMode: true, rounding: 'Math.round' })

Or:

function round(in){ return Math.round(in) }
const debugCompiled = fastJson(schema, { debugMode: true, rounding: round })

Where

-  customAsInteger = customAsInteger.replace('parseInt', options.rounding)
+ customAsInteger = customAsInteger.replace('parseInt', options.rounding.name)

Which one do you think would be better? Or do you see nicest solutions?

Checklist

@mcollina
Copy link
Member

mcollina commented Jan 6, 2021

I would support only strings: 'round', 'floor', 'ceil'.

@jsumners
Copy link
Member

jsumners commented Jan 6, 2021

There isn't any concept of "rounding" defined in the spec https://json-schema.org/draft/2019-09/json-schema-validation.html#rfc.section.6.2

@mcollina
Copy link
Member

mcollina commented Jan 6, 2021

so you think the default behavior should be to throw if it's not an integer?

@jsumners
Copy link
Member

jsumners commented Jan 6, 2021

I believe that is the consensus we reached the last time we discussed how to handle non-spec compliant values (i.e. undefined).

@mcollina
Copy link
Member

mcollina commented Jan 6, 2021

I would actually prefer to not do anything in this case if we want to throw.

@jsumners
Copy link
Member

jsumners commented Jan 6, 2021

Looking over this again, it seems this PR is making use of an undocumented second parameter to the FJS function: const stringifierFunction = FJS(jsonSchema, {...options}). So I am okay with this PR since it isn't adding new syntax to the JSON Schema spec. However, this PR should also update the documentation for this mysterious options object or there should be a fast follow-up PR that does so.

@Eomm
Copy link
Member Author

Eomm commented Jan 6, 2021

Yes, this pr doesn't force the user to add non-spec fields to the input schema.

This pr force the type coertion of those properties defined by user as "integer"

The second parameter is documented to add external schemas and debug the stringify function, but I agree on a follow up pr to document better this option that is spread across the readme and I will take care of it 👍🏼

@Eomm
Copy link
Member Author

Eomm commented Jan 14, 2021

Are there any concerns for this PR?

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina merged commit a0d0b4b into fastify:master Jan 15, 2021
@Eomm Eomm mentioned this pull request Jan 15, 2021
4 tasks
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.

Integer are not truncated

3 participants