Adding 'new_record' field for all schemas #11
Conversation
| } |
There was a problem hiding this comment.
@david-caro, do we care about EOL here?
There was a problem hiding this comment.
Actually this would be solved by also transferring my little schema-linters tools: https://github.com/inspirehep/inspire-next/blob/master/scripts/prettify_json
There was a problem hiding this comment.
I don't really care, I see no reason to complain about it, but if you give me a reason to avoid it I'll follow, automating as @kaplun says would be the best though.
There was a problem hiding this comment.
Ok. I asked because it looked like you took care of avoiding EOLs, so I wasn't sure if there was a reason for that. I don't care either, they can stay.
There was a problem hiding this comment.
I think it's what the js script generated 'as is' :)
|
@spirosdelviniotis in the commit log, please explain in the bullet list what the change is about in human terms. |
spirosdelviniotis
force-pushed
the
1609-supporting_merging_records-feature
branch
from
64ab662
to
bee1ba3
Compare
|
@david-caro can you take a look? |
| @@ -115,6 +115,9 @@ | |||
| "value": ")qk,t/ [((>?! oOEE=a8uIb\"u5V_J@/%qmn&YH,:1:stA@z}TNi/lrF{*X;YBt-e=J*AFmNsTF_(4[dtSFMpIWt1*:pA, UZ~oG)`hd5AYA.FW@M-Wq3+ag7a\\:J~4(3aIM7mbYg;5yOSyv,S?Az`ncWRH{cBAv`L%jEsmY" | |||
| }, | |||
| "native_name": [], | |||
| "new_record": { | |||
| "$ref": "1M\"?!ouZ]W}^ 6ZBtT (OpV2~FoO1*YXan`6q+Zz{|E}b&Rty<P]6o0i)l[,YDwm4_9c}HnG.:l_H" | |||
There was a problem hiding this comment.
Something got currputed here...
There was a problem hiding this comment.
Ah no. I see that this is an example fixture... :)
There was a problem hiding this comment.
@kaplun is right about the commit message, we wrote it too short, can you do a small explanation of 'why' the change is needed? (I know that the explanation is already in the issue that it's specified, but you can't get the info of the issue from git log itself...).
Also replace the '#XXX' in the commit message by inspirehep/inspire-next#1609, so the issue is linked properly, as leaving just #1609 means the issue number 1609 on the current repo and not inspire-next. This will require a small change in the changelog generator to show the bug id I think (to do in a different pr).
* Makes `scripts/generate_example_records.js` work out of the box by adding the proper requires and making it executable (closes inspirehep#12). * Also adds `node_modules` to `.gitignore` so that we can `npm install json-schema-faker` locally. Signed-off-by: Jacopo Notarstefano <jacopo.notarstefano@cern.ch>
Signed-off-by: Jacopo Notarstefano <jacopo.notarstefano@cern.ch>
spirosdelviniotis
force-pushed
the
1609-supporting_merging_records-feature
branch
2 times, most recently
from
52203eb
to
d9a1645
Compare
* NEW Adds 'new_record' field for all the json schemas regarding merged records. * Addresses inspirehep/inspire-next#1609. Signed-off-by: Spiros Delviniotis <spyridon.delviniotis@cern.ch>
spirosdelviniotis
force-pushed
the
1609-supporting_merging_records-feature
branch
from
d9a1645
to
afe76b2
Compare
|
@spirosdelviniotis You're making a bit of a mess with the commits here : ) I'll take care of merging this for you. |
| "new_record": { | ||
| "$ref": "elements/json_reference.json", | ||
| "description": "Master record that replaces this record", | ||
| "title": "New record" |
There was a problem hiding this comment.
For now these will stay, but I really don't like titles that repeat the field name, and descriptions which leave me more confused than before (what is a master record?).
There was a problem hiding this comment.
I think @spirosdelviniotis copied from the original schema (hep) that already had this field, beside being this $ref the description and title fields will be overwritten by whatever referenced (a bit of a sad functionality of jsonschema).
There was a problem hiding this comment.
beside being this $ref the description and title fields will be overwritten by whatever referenced (a bit of a sad functionality of jsonschema).
Uh, I didn't know that!
| @@ -1,6 +1,8 @@ | |||
| #!/usr/bin/env node | |||
| //requires the json-schema-faker npm package installed | |||
| var jsf = require('json-schema-faker') | |||
| var fs = require('fs', | |||
There was a problem hiding this comment.
The fs and path were already loaded by node on my installation when running it, sorry if it did not worked for you.
There was a problem hiding this comment.
Argh! I wonder how that happened...
Signed-off-by: Spiros Delviniotis spyridon.delviniotis@cern.ch