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.

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

Bulk saving for metadata and exposing documentation API #314

Merged
merged 16 commits into from
May 6, 2020

Conversation

zikolach
Copy link
Contributor

@zikolach zikolach commented Apr 16, 2020

Allow to update multiple metadata categories at once.

Important: based on PR iiasa/ixmp_source#290

PR checklist

  • Tests added.
  • Documentation added.
  • Release notes updated.

@codecov
Copy link

codecov bot commented Apr 16, 2020

Codecov Report

Merging #314 into master will decrease coverage by 0.04%.
The diff coverage is 98.83%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #314      +/-   ##
==========================================
- Coverage   97.62%   97.57%   -0.05%     
==========================================
  Files          42       42              
  Lines        4377     4454      +77     
==========================================
+ Hits         4273     4346      +73     
- Misses        104      108       +4     
Impacted Files Coverage Δ
ixmp/backend/jdbc.py 94.66% <97.29%> (-0.51%) ⬇️
ixmp/backend/base.py 98.69% <100.00%> (+0.05%) ⬆️
ixmp/core.py 95.67% <100.00%> (+0.04%) ⬆️
ixmp/tests/backend/test_base.py 98.36% <100.00%> (+0.08%) ⬆️
ixmp/tests/backend/test_jdbc.py 100.00% <100.00%> (ø)
ixmp/tests/core/test_scenario.py 100.00% <100.00%> (ø)
ixmp/backend/io.py 98.56% <0.00%> (+0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update febde8a...0a873c0. Read the comment docs.

@zikolach zikolach changed the title Allow saving multiple metadata entries in one go Bulk saving for metadata and exposing documentation API Apr 17, 2020
@zikolach zikolach force-pushed the feature/metadata-bulk-update branch from 493d3a7 to 250a27d Compare April 17, 2020 14:18
@zikolach zikolach changed the base branch from jdbc-memory-mgmt to master April 17, 2020 14:19
@zikolach zikolach marked this pull request as ready for review April 17, 2020 14:29
Copy link
Member

@khaeru khaeru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zikolach thanks for these additions. As a first set of requested changes:

  • Documentation of the Backend API goes on the abstract methods in base.Backend. The implementations of these methods on jdbc.JDBCBackend generally have no docstring, unless they have significant differences in behaviour from the general API which need explanation. See the existing code.
  • Please update doc/source/api-backend.rst to refer to the new methods.

ixmp/backend/jdbc.py Outdated Show resolved Hide resolved
ixmp/backend/jdbc.py Outdated Show resolved Hide resolved
ixmp/backend/jdbc.py Show resolved Hide resolved
ixmp/backend/jdbc.py Outdated Show resolved Hide resolved
ixmp/core.py Outdated
----------
name_or_names : str or list
If the argument is list, it used to remove multiple metadata
entries at once. Otherwise, use the argument
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check wrapping.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what you mean as wrapping

@zikolach zikolach force-pushed the feature/metadata-bulk-update branch from e943c98 to 6c37514 Compare April 23, 2020 20:25
@zikolach
Copy link
Contributor Author

@khaeru thanks for comments! Please have a look at 6c37514 - it should be fixed there

Copy link
Member

@danielhuppmann danielhuppmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a few minor comments on how to name arguments and on docstrings...

ixmp/backend/base.py Outdated Show resolved Hide resolved
ixmp/backend/base.py Outdated Show resolved Hide resolved
ixmp/backend/base.py Outdated Show resolved Hide resolved
@@ -55,6 +55,8 @@ class Platform:
_backend_direct = [
'open_db',
'close_db',
'get_doc',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused, is there now a top-level function to get or set docstrings?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, methods of backend class which are included into _backend_direct are exposed directly as methods of Platform

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, thanks for the clarification - how is this rendered in the docs? Is there an example where I can see this already?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question! I think it is not available on Platform class documentation.

@khaeru what do you think, can we adjust __doc__ attribute to incorporate those backend methods documentation into Platform docs?

ixmp/backend/jdbc.py Outdated Show resolved Hide resolved
ixmp/backend/jdbc.py Outdated Show resolved Hide resolved
ixmp/backend/jdbc.py Outdated Show resolved Hide resolved
ixmp/backend/jdbc.py Outdated Show resolved Hide resolved
@zikolach zikolach force-pushed the feature/metadata-bulk-update branch from cdc43f6 to 99404d1 Compare May 5, 2020 11:03
Copy link
Member

@danielhuppmann danielhuppmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @zikolach, looks great! I think it's fine to merge this and tackle the question how backend-functions will be displayed in the docs in a subsequent PR.

@zikolach
Copy link
Contributor Author

zikolach commented May 5, 2020

* Documentation of the Backend API goes on the abstract methods in `base.Backend`. The implementations of these methods on `jdbc.JDBCBackend` generally have no docstring, unless they have significant differences in behaviour from the general API which need explanation. See the existing code.

* Please update `doc/source/api-backend.rst` to refer to the new methods.

@khaeru thanks for suggestions. I moved docstrings to backend base class and added new methods to api-backend.rst.

@danielhuppmann
Copy link
Member

I moved docstrings to backend base class and added new methods to api-backend.rst.

Does this mean these functions will not be included in the Platform docs? If that's the case, I don't think that this is a good strategy...

@zikolach
Copy link
Contributor Author

zikolach commented May 5, 2020

Does this mean these functions will not be included in the Platform docs

It depends on what you mean as Platform docs. These methods docstrings are added to Backend documentation, not to Platform class documentation.

@zikolach zikolach requested a review from khaeru May 5, 2020 13:09
@danielhuppmann
Copy link
Member

This reference to the new functions is not understandable to anyone who was not part of this discussion.

The methods ~.base.Backend.open_db, ~.base.Backend.close_db, ~.base.Backend.get_doc, and ~.base.Backend.set_doc may also be called via Platform.

It's not obvious how to call these functions - Platform.base.Backend.open_db?

@khaeru
Copy link
Member

khaeru commented May 6, 2020

This reference to the new functions is not understandable […]

The methods ~.base.Backend.open_db, ~.base.Backend.close_db, ~.base.Backend.get_doc, and ~.base.Backend.set_doc may also be called via Platform.

See the Sphinx docs:

If you prefix the content with ~, the link text will only be the last component of the target. For example, :py:meth:'~Queue.Queue.get' will refer to Queue.Queue.get but only display get as the link text.

Sphinx renders the sentence as "The methods open_db(), close_db(), …" with links.

If that doesn't suit a particular use case/target user/story, other alternatives are:

  • Use the Sphinx .. py:function:: directive to put the user-facing documentation in api-python.rst.
  • Don't use _backend_direct (originally introduced as a convenience when developing the Backend API), and write short (possibly one-line) methods. See for example add_unit/units, set_log_level/get_log_level. In this case, observe separation of concerns as described in the docs; e.g. just as set_log_level raises ValueError for invalid input, set_doc should raise ValueError for an invalid domain.

@zikolach zikolach force-pushed the feature/metadata-bulk-update branch from 811cd7c to 129decc Compare May 6, 2020 10:16
@zikolach zikolach merged commit d585804 into master May 6, 2020
@zikolach zikolach deleted the feature/metadata-bulk-update branch May 6, 2020 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants