-
Notifications
You must be signed in to change notification settings - Fork 14
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
Addition of json_source module #49
Conversation
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.
Amazing work regarding the removal of code duplication! Very happy to see the code passing pylint. I think some restructuring is still needed. Also please squash you commits into one when resubmitting.
src/alexandria3k/json_source.py
Outdated
# pylint: disable=invalid-name | ||
|
||
|
||
class JSONElementsCursor(ElementsCursor): |
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 can't see how this class is related to JSON. Maybe it should be in data_source.py
with a name such as NestedElementsCursor
? Given the class inheritance, please think hard regarding the appropriate names of all (new and existing) classes. Feel free to send me a diagram and discuss this. You can also send me a simple class diagram to discuss ideas.
Please also expand the docstring to explain what this cursor expects and what functionality it provided.
src/alexandria3k/json_source.py
Outdated
self.elements = None | ||
|
||
|
||
class RecordsCursor(JSONElementsCursor): |
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.
The comments for JSONElementsCursor
also apply here, right?
I moved the two cursor classes to the |
Add json_source module with JSONElementsCursor and RecordsCursor Adjust docstring in json_source Move NestedElementsCursor and RecordsCursor to data_source
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.
Amazing, well done! As a reminder, please send a PR to update docs/schema/datacite.dot and another one for an example (e.g. extracting metrics) under examples/
. I'd be happy to do the second one if you prefer.
The
json_source
module contains:JSONElementsCursor
, which is identical to both howCrossrefElementsCursor
andDataciteElemenrsCursors
were andRecordsCursor
, which contains the methods from theWorksCursor
's classes from the Crossref and the DataCite modules, with an exception of thecurrent_row_value
method which called for an abstract implementation