Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Addition of json_source module #49

Merged
merged 1 commit into from
Jun 26, 2024
Merged

Addition of json_source module #49

merged 1 commit into from
Jun 26, 2024

Conversation

evgepab
Copy link
Contributor

@evgepab evgepab commented Jun 26, 2024

The json_source module contains:

  • JSONElementsCursor, which is identical to both how CrossrefElementsCursor and DataciteElemenrsCursors were and
  • RecordsCursor, which contains the methods from the WorksCursor 's classes from the Crossref and the DataCite modules, with an exception of the current_row_value method which called for an abstract implementation

dspinellis
dspinellis requested changes Jun 26, 2024
View reviewed changes
Copy link
Owner

@dspinellis dspinellis left a comment •
edited
Loading

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/data_sources/crossref.py Outdated Show resolved Hide resolved
src/alexandria3k/json_source.py Outdated
# pylint: disable=invalid-name


class JSONElementsCursor(ElementsCursor):
Copy link
Owner

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):
Copy link
Owner

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?

@evgepab
Copy link
Contributor Author

evgepab commented Jun 26, 2024

I moved the two cursor classes to the data_source module, renamed the first cursor to NestedElementsCursor and tried to make their functionalities clearer through their docstrings.

@evgepab
Resolve RightsCursor's Rowid
50ac85c 
Add json_source module with JSONElementsCursor and RecordsCursor

Adjust docstring in json_source

Move NestedElementsCursor and RecordsCursor to data_source
dspinellis
dspinellis approved these changes Jun 26, 2024
View reviewed changes
Copy link
Owner

@dspinellis dspinellis left a 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.

@dspinellis dspinellis merged commit 4727535 into dspinellis:datacite Jun 26, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dspinellis dspinellis dspinellis approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@evgepab @dspinellis