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

Replace custom configuration definition with standard approach #244

Merged
merged 2 commits into from
Oct 12, 2023

Conversation

mih
Copy link
Member

@mih mih commented Oct 10, 2023

Since v0.16 datalad provides a register_config() helper for extensions to inject their own configuration into the central registry.

This changeset replaces the previous custom implementation with this standard approach. It has not consequences re user behavior.

An additional benefit is that this configuration is now also user-accessible, including a basic documentation:

>>> import datalad_container
>>> from datalad.api import configuration
>>> configuration('dump', 'datalad.containers.location')
# Container location
# path within the dataset where to store containers
# Value constraint: str
datalad.containers.location=.datalad/environments
[{'action': 'dump_configuration',
  'refds': '/home/mih/hacking/datalad/container',
  'name': 'datalad.containers.location',
  'value': '.datalad/environments',
  'purpose': 'Container location',
  'description': 'path within the dataset where to store containers',
  'value_type': constraint:str,
  'path': '/home/mih/hacking/datalad/container',
  'status': 'ok'}]

Closes #239

Since v0.16 datalad provides a `register_config()` helper for extensions
to inject their own configuration into the central registry.

This changeset replaces the previous custom implementation with this
standard approach. It has not consequences re user behavior.

An additional benefit is that this configuration is now also
user-accessible, including a basic documentation:

```py
>>> import datalad_container
>>> from datalad.api import configuration
>>> configuration('dump', 'datalad.containers.location')
datalad.containers.location=.datalad/environments
[{'action': 'dump_configuration',
  'refds': '/home/mih/hacking/datalad/container',
  'name': 'datalad.containers.location',
  'value': '.datalad/environments',
  'purpose': 'Container location',
  'description': 'path within the dataset where to store containers',
  'value_type': constraint:str,
  'path': '/home/mih/hacking/datalad/container',
  'status': 'ok'}]
```

Closes datalad#239
@codeclimate
Copy link

codeclimate bot commented Oct 10, 2023

Code Climate has analyzed commit 8ca639e and detected 0 issues on this pull request.

View more on Code Climate.

@codecov
Copy link

codecov bot commented Oct 10, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (450b9a8) 94.74% compared to head (8ca639e) 94.74%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #244   +/-   ##
=======================================
  Coverage   94.74%   94.74%           
=======================================
  Files          25       24    -1     
  Lines        1104     1104           
=======================================
  Hits         1046     1046           
  Misses         58       58           
Files Coverage Δ
datalad_container/__init__.py 100.00% <100.00%> (ø)
datalad_container/containers_add.py 89.74% <ø> (-0.07%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@yarikoptic yarikoptic added the internal Changes only affect the internal API label Oct 12, 2023
mih added a commit to mih/datalad-container that referenced this pull request Oct 12, 2023
This causes worse portability issues than the previous approach
of storing `python(.exe)`.

Instead pass the placeholder on to `datalad run`.

This would now error with

```
command has an unrecognized placeholder: 'python'
```

and requires a further intervention. Three options:

- have datalad-core define
  `datalad.run.substitutions.python=sys.executable`
- wait for datalad#244
  and use `register_config()` to have datalad-container define it
- have datalad-container define it at `containers-run` runtime by
  patching the configuration for the execution time (would have `rerun`
  fail still)
- add the configuration item to the committed dataset config

Closes datalad#249
@yarikoptic yarikoptic merged commit 2fc957f into datalad:master Oct 12, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal Changes only affect the internal API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Start using register_config()
2 participants