This repository was archived by the owner on Oct 15, 2024. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
RFQ-T Firm Quotes #162
RFQ-T Firm Quotes #162
Changes from all commits
d3cfe8e
05ca1b6
e026de2
0a0de81
89ff735
61cbd5c
268bff8
2578a79
6b21a39
d11b850
b126316
cbecfc4
902b15d
bdc7bae
47227ad
0255b25
29d6215
b0488ef
e50fc98
e150eec
7c448d3
08f4d31
f325f42
ea8c8e9
0633aca
556a45b
e1693bb
11cb5a8
bb2814a
74d3ea7
6a0aed8
fb176a5
2e7c7bb
3ed896f
c772d43
735fe3e
bf835d0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use a
boolean
type here? http://json-schema.org/understanding-json-schema/reference/boolean.htmlThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to. I tried.
jsonschema
choked on it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At test runtime i get this json schema error
{\"field\":\"instance.intentOnFilling\",\"code\":1001,\"reason\":\"is not of a type(s) boolean\"}
This happened no matter whether my client said
quote?intentOnFilling
orquote?intenOnFilling=true
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it.
I think this schema could be cleaner if we:
flag
functionality (i.e. setting?intentOnFilling
indicating thatintentOnFilling
is equal totrue
). I have not seen this used in query parameters before, and IMHO is not super important to supportenum
type to force the consumer of the API to specify the texttrue
,false
for our 'boolean' params (intentOnFilling, skipValidaiton). (See https://stackoverflow.com/a/16826238 and https://json-schema.org/understanding-json-schema/reference/generic.html#enumerated-valuesThis would allow
?skipValidation=truue
to raise a schema error instead of silently being set tofalse
on our backend.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How's this: bb2814a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to add a comment describing what's happening here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in 03f7673
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍