Change Summary:
Naming schemes are difficult
Summary: |
|
---|
Review Request #392 — Created Nov. 9, 2015 and discarded
Information | |
---|---|
smillernl | |
Lunr | |
feature/json_validation | |
Reviewers | |
lunr | |
Corona: JSON validation
Description | From | Last Updated |
---|---|---|
This extra locator parameter looks out of place :/ Just a thought: Would it be an option to have this ... | tardypad |
src/Lunr/Corona/JsonView.php (Diff revision 1) |
---|
First I don't think the verification should be done only on a 200 return code
Some webservices might return useful data using a different code
The question is whether we should do it for all status codes or maybe just 2xx ones.Second, deciding to do the verification only if the schema file exists, is not flexible enough.
We will have schema files defined there to be used by the clients only as a source of information
Those should not be used for verification (processing too long or other reason)
And we don't want to duplicate those schema files (one place for verification, one for documentation)
I would go for configuration usage here
src/Lunr/Corona/JsonView.php (Diff revision 1) |
---|
you should not override the message here
there is most likely more useful information in there already
src/Lunr/Corona/JsonView.php (Diff revision 1) |
---|
This is not related to JSON validation, that needs to be in a different review
Already few comments about it though:
- I would change the variable name from $time to $initial_time or such (to be more explicit)
- why only for web requests and not cli?
Branch: |
|
||||
---|---|---|---|---|---|
Commit: |
|
||||
Diff: |
Revision 2 (+98 -15) |
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 3 (+84 -4) |
src/Lunr/Corona/JsonView.php (Diff revision 3) |
---|
This extra locator parameter looks out of place :/
Just a thought:
Would it be an option to have this validation feature as part of a child class like JsonValidationView?So that we don't have this extra parameter and we don't break the class either
by replacing all constructor parameter with only the locatorThat would then leave each individual project to implement the way
they want to choose which view to use (whether via config, json schema file existence, ...).The only thing needed in that JsonView class would be like a hook extra_result_process(&$json)
called before the output, that would do nothing by default
(come with a better name than this :)