This repository has been 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 27 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 just do something like the following?
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.
I did some digging when I updated the schemas here. I had initially wanted to make them
type: "boolean"
(whatever exactly that would mean!) but thejsonschema
package didn't handle that nicely. Later discovered this question which is a good analog to what we're doing here: OAI/OpenAPI-Specification#1782I changed this here because I liked the idea of the mere presence of this flag indicating it should be true, eg
/quote?intentOnFilling&...
.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 we resolve #162 (comment) will impact this
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.
Why do we want to consider
req.query.skipValidation === ''
as 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.
Same logic behind my recent change to
intentOnFilling
. I think it makes logical sense to be able to say just/quote?skipValidation&...
, and when that's the query string,req.query
here contains"skipValidation": ""
.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.
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.
👍