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

Expose list_docs() in the facade & enable list_docs(id__in=...) #167

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

glatterf42
Copy link
Member

Here's the PR you asked for, @danielhuppmann: it exposes the list() functionality we already have for docs for most facade objects and enables a filter so that only IDs from a provided list are considered.

There are several things to consider, though:

  1. I don't know if this is how you envisioned the facade API (i.e. providing IDs; the other docs functions often translate one name to one ID first, which would need to be adapted for a list of names).
  2. The PR comes with tests, but these are not exhaustive. For instance, I don't enforce anywhere that only one of dimension_id or dimension_id__in can be used. In fact, if both are used, we silently discard the latter at the moment. This behavior is not tested.
  3. The code includes quite a bit of repetition, which is not great style/potentially even technical debt. However, I didn't see how else to do this unless I'm rewriting more significant parts of the docs feature, which might be worthwhile itself, but not the target here.
  4. Currently, the optimization items in the facade layer all have a .docs property through which one can manage each item's docs, but their repository classes lack any docs functionality. So rather than adding all of that here, I assumed we don't urgently need this and thus did not expose the list_docs() function for them.

@glatterf42 glatterf42 added the enhancement New feature or request label Mar 4, 2025
@glatterf42 glatterf42 self-assigned this Mar 4, 2025
@glatterf42 glatterf42 force-pushed the enh/list-docs-with-ids branch from 5a3d04d to f4c163f Compare March 4, 2025 15:38
Copy link

codecov bot commented Mar 4, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.1%. Comparing base (c7ea1f3) to head (f4c163f).

Additional details and impacted files
@@          Coverage Diff          @@
##            main    #167   +/-   ##
=====================================
  Coverage   87.0%   87.1%           
=====================================
  Files        230     230           
  Lines       8170    8198   +28     
=====================================
+ Hits        7112    7141   +29     
+ Misses      1058    1057    -1     
Files with missing lines Coverage Δ
ixmp4/core/iamc/variable.py 84.8% <100.0%> (+0.5%) ⬆️
ixmp4/core/model.py 84.8% <100.0%> (+0.5%) ⬆️
ixmp4/core/region.py 83.1% <100.0%> (+0.4%) ⬆️
ixmp4/core/scenario.py 84.8% <100.0%> (+0.5%) ⬆️
ixmp4/core/unit.py 87.2% <100.0%> (+0.3%) ⬆️
ixmp4/data/abstract/docs.py 80.0% <100.0%> (+1.0%) ⬆️
ixmp4/data/api/base.py 88.4% <ø> (ø)
ixmp4/data/api/docs.py 96.1% <100.0%> (+4.1%) ⬆️
ixmp4/data/db/base.py 92.3% <100.0%> (+<0.1%) ⬆️
ixmp4/data/db/docs.py 97.2% <100.0%> (+0.2%) ⬆️
... and 4 more

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant