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

New config manager #760

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

New config manager #760

wants to merge 7 commits into from

Conversation

mih
Copy link
Member

@mih mih commented Sep 25, 2024

TODO:

Remaining issues for replacing the legacy ConfigManager with the actual new LegacyConfigManager

FAILED datalad_next/patches/tests/test_cli_configoverrides.py::test_cli_configoverrides - AttributeError: 'mappingproxy' object has no attribute 'update'
FAILED datalad_next/annexremotes/tests/test_archivist.py::test_archivist_retrieval - datalad.runner.exception.CommandError: CommandError: 'git -c diff.ignoreSubmo...

@mih mih force-pushed the configman branch 3 times, most recently from 2d48c2d to 2f0d377 Compare September 25, 2024 08:51
Copy link

codecov bot commented Sep 25, 2024

Codecov Report

Attention: Patch coverage is 95.46926% with 42 lines in your changes missing coverage. Please review.

Project coverage is 23.07%. Comparing base (2f6cb8e) to head (1219605).

Files with missing lines Patch % Lines
datalad_next/config/legacy.py 90.17% 10 Missing and 12 partials ⚠️
datalad_next/config/git.py 90.19% 5 Missing and 5 partials ⚠️
datalad_next/config/default.py 86.04% 4 Missing and 2 partials ⚠️
datalad_next/config/tests/test_core.py 99.22% 1 Missing and 2 partials ⚠️
datalad_next/config/item.py 96.66% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main     #760       +/-   ##
===========================================
- Coverage   79.72%   23.07%   -56.66%     
===========================================
  Files         185      195       +10     
  Lines       12410    13333      +923     
  Branches     2162     2447      +285     
===========================================
- Hits         9894     3076     -6818     
- Misses       2263    10175     +7912     
+ Partials      253       82      -171     

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

Previously, the setup was confusing. Every symbol imported to module
level was documented directly. If a ToC was added, sphinx would fail
due to documentation generated twice for the same symbol.

The new setup **requires** a dedicated autosummary selection of
documented content.

I think this is better. Often modules provide a lot of stuff, and
direct documentation rendering on a single page is overwhelming.
The explicit selection also allows for items to be excluded
that are only there for technical or historical reasons. This allows for
an overall more accessible documentation.
@mih mih force-pushed the configman branch 3 times, most recently from 2ebc8bc to f861e76 Compare September 26, 2024 07:35
We have no other choice, but to modify them. Even for the legacy
adaptor, we do not want to maintain senseless complexities.
The changes in the tests will document the exact differences.
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.

1 participant