-
Notifications
You must be signed in to change notification settings - Fork 26
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor the field_categories #44
Conversation
3611e03
to
519b13d
Compare
0d43f68
to
485cb31
Compare
Ready for review! @david-caro |
|
||
|
||
function resolve_schema(unresolved_schema, base_path, definitions) { | ||
definitions = set_definitions(unresolved_schema['definitions'], definitions); |
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 would use select/pick instead of set for the name of the method, as usually the methods that have 'set' on the name are expected to modify on of the parameters or some global, setting a property on them.
delete(resolved_element_schema['$schema']) | ||
return resolved_element_schema | ||
} else if (key === '$ref' && typeof(unresolved_schema[key]) === 'string' && unresolved_schema[key][0] === '#') { | ||
return definitions |
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.
shouldn't this return resolve_schema(extract_definition(definitions, defs_path), schema_path, definitions)
? Where the defs_path is the path part of the '#key1/key2' and the extract_definiton
just extracts the value for the key it references?
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.
Is it possible to have a $ref
statement in the definitions obj?
485cb31
to
ba6bac0
Compare
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.
Not critical at all though
|
||
|
||
function resolve_schema(unresolved_schema, base_path, definitions) { | ||
definitions = pick_definitions(unresolved_schema['definitions'], definitions); |
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.
definitions = unresolved_schema['definitions'] || definitions
|
||
|
||
function extract_definition(definitions_object, defs_path) { | ||
keys_array = defs_path.split('/'); |
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.
var keys_array...
fd433e9
to
f59e85c
Compare
I think you can merge! |
f59e85c
to
98cc337
Compare
you should rebase on latest master too, to get rid of that extra commit that's shown. |
d37d14f
to
2283686
Compare
@david-caro rebased and tests have passed. |
* INCOMPATIBLE split field_categories in 2 new fields. * Remove field_categories field. * Add inspire_field_categories. * Add external_field_categories.
2283686
to
dd57da6
Compare
This change is:
field_categories
external_field_categories
andinspire_field_categories
In order to maintain different lists for the categories of INSPIRE and the categories that we receive from external sites, during the articles collection