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

Dataset configuration management via git-config calls #744

Merged
merged 11 commits into from
Sep 1, 2016
Merged

Dataset configuration management via git-config calls #744

merged 11 commits into from
Sep 1, 2016

Conversation

mih
Copy link
Member

@mih mih commented Aug 30, 2016

No description provided.

@mih mih mentioned this pull request Aug 30, 2016
Closed
@coveralls
Copy link

coveralls commented Aug 30, 2016 •
edited
Loading

Coverage Status

Coverage increased (+0.05%) to 86.626% when pulling 65570c6 on hanke:nf-dsconfig into e1f55ec on datalad:master.

@codecov-io
Copy link

codecov-io commented Aug 30, 2016 •
edited by codecov bot
Loading

Codecov Report

Merging #744 into master will increase coverage by 0.16%.
The diff coverage is 98.79%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #744      +/-   ##
==========================================
+ Coverage    86.6%   86.77%   +0.16%     
==========================================
  Files         195      198       +3     
  Lines       17640    17884     +244     
==========================================
+ Hits        15277    15518     +241     
- Misses       2363     2366       +3
Impacted Files Coverage Δ
datalad/tests/utils.py 91.34% <100%> (+0.05%) ⬆️
datalad/config.py 90.27% <100%> (ø) ⬆️
datalad/distribution/dataset.py 94.82% <100%> (+0.18%) ⬆️
datalad/distribution/tests/test_dataset_config.py 100% <100%> (ø)
datalad/distribution/create.py 92.18% <100%> (+0.52%) ⬆️
datalad/distribution/tests/test_create.py 100% <100%> (ø) ⬆️
datalad/support/tests/test_dsconfig.py 100% <100%> (ø)
datalad/support/dsconfig.py 97.77% <97.77%> (ø)
... and 1 more

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 64730e6...387b2e2. Read the comment docs.

yarikoptic
yarikoptic reviewed Aug 30, 2016
View reviewed changes
datalad/config.py
@@ -129,7 +129,7 @@ def _get_default_file_candidates(self):
cfg_file_candidates.append(opj(home_cfg_base_path, 'datalad.cfg'))

# current dir config
cfg_file_candidates.append(opj('.datalad', 'config'))
cfg_file_candidates.append('datalad.cfg')
Copy link
Member

Choose a reason for hiding this comment

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

as talked elsewhere, let's keep .datalad/config and better fix the docs, than the code

Copy link
Member Author

Choose a reason for hiding this comment

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

It does not work. This parser requires Python's format. But a dataset's .datalad/config is in Git's format.

Copy link
Member

Choose a reason for hiding this comment

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

ah right... since we are aiming for unification and don't think anyone would have those datalad.cfg laying around -- just comment it out entirely I guess. later it should read .datalad/config right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes.

On Tue, Aug 30, 2016 at 3:27 PM, Yaroslav Halchenko <
notifications@github.com> wrote:

In datalad/config.py
#744 (comment):

@@ -129,7 +129,7 @@ def _get_default_file_candidates(self):
cfg_file_candidates.append(opj(home_cfg_base_path, 'datalad.cfg'))

     # current dir config

  •    cfg_file_candidates.append(opj('.datalad', 'config'))

  •    cfg_file_candidates.append('datalad.cfg')

ah right... since we are aiming for unification and don't think anyone
would have those datalad.cfg laying around -- just comment it out entirely
I guess. later it should read .datalad/config right?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/datalad/datalad/pull/744/files/65570c6d158c41afcab243aca653d2b3b08e833e#r76793811,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAIVH8Vn2ODoeYD5mvcJOnYKgynbPCKyks5qlC_DgaJpZM4JwXjp
.

Michael Hanke
http://mih.voxindeserto.de

@yarikoptic
Copy link
Member

please merge/rebase on top of current master to make nd90 happy again

@mih mih force-pushed the nf-dsconfig branch 2 times, most recently from 25b2396 to baea9c3 Compare August 30, 2016 13:45
@mih
Copy link
Member Author

mih commented Aug 30, 2016

Rebased.

@mih mih force-pushed the nf-dsconfig branch 3 times, most recently from 25d0c04 to 781a155 Compare August 30, 2016 18:04
@mih mih changed the title WiP: Currently a mess, but also has ConfigParser implementation (bits) Dataset configuration management via git-config calls Aug 30, 2016
@mih
NF: Draft of our own config parser
983da78 
GitPython's is complex and cannot handle multi-value options
@mih
Copy link
Member Author

mih commented Aug 30, 2016

@bpoldrack @yarikoptic I think this is now a reasonably functional draft. Of course many more things could be implemented...

I still needs some docs. Maybe later today.

@mih
Copy link
Member Author

mih commented Aug 30, 2016

@yarikoptic When this gets merged, I'll replace the then obsolete code in nf-metaparser with this and should be able to complete (and fix) the current draft implementation.

@coveralls
Copy link

coveralls commented Aug 30, 2016 •
edited
Loading

Coverage Status

Coverage increased (+0.1%) to 86.736% when pulling 39f6a17 on hanke:nf-dsconfig into 64730e6 on datalad:master.

@coveralls
Copy link

coveralls commented Aug 30, 2016 •
edited
Loading

Coverage Status

Coverage increased (+0.1%) to 86.736% when pulling 39f6a17 on hanke:nf-dsconfig into 64730e6 on datalad:master.

@coveralls
Copy link

coveralls commented Aug 30, 2016 •
edited
Loading

Coverage Status

Coverage increased (+0.1%) to 86.736% when pulling 39f6a17 on hanke:nf-dsconfig into 64730e6 on datalad:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 86.736% when pulling 39f6a17 on hanke:nf-dsconfig into 64730e6 on datalad:master.

@coveralls
Copy link

coveralls commented Aug 30, 2016 •
edited
Loading

Coverage Status

Coverage increased (+0.1%) to 86.736% when pulling 39f6a17 on hanke:nf-dsconfig into 64730e6 on datalad:master.

@mih mih mentioned this pull request Aug 31, 2016
Merged
@coveralls
Copy link

coveralls commented Aug 31, 2016 •
edited
Loading

Coverage Status

Coverage increased (+0.1%) to 86.752% when pulling 8b00ed6 on mih:nf-dsconfig into 64730e6 on datalad:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 86.752% when pulling 8b00ed6 on mih:nf-dsconfig into 64730e6 on datalad:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 86.752% when pulling 8b00ed6 on mih:nf-dsconfig into 64730e6 on datalad:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 86.752% when pulling 8b00ed6 on mih:nf-dsconfig into 64730e6 on datalad:master.

yarikoptic
yarikoptic reviewed Sep 1, 2016
View reviewed changes
datalad/distribution/create.py
# make sure we reset this variable completely, in case of a re-create
ds.config.unset('datalad.annex.origin', where='dataset')
ds.config.add('datalad.annex.origin', vcs.uuid, where='dataset')
ds.repo.add('.datalad', git=True)
Copy link
Member

Choose a reason for hiding this comment

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

btw what happens to it when merging multiple datasets/repos? one to be chosen or a new one created or ... ? just this tracking of the origin and siblings -- still not feeling where the benefit would come

Copy link
Member Author

Choose a reason for hiding this comment

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

That needs human attention. Impossible to infer what would be appropriate.

@yarikoptic
Copy link
Member

Since _ is not legit char for option in git config, I guess I will replace current e.g.

$> cat .datalad/crawl/crawl.cfg 
[crawl:pipeline]
template = crcns
_dataset_category = challenges
_dataset = ch-epfl-2009
_leading_dirs_depth = 0
_rename=/crcns-ch-epfl-2009-/

with a section in .datalad/config:

[datalad "crawl.pipeline"]
template = crcns
arg-dataset-category = challenges
arg-dataset =  ch-epfl-2009
arg-leading-dirst-depth = 0
arg-rename = /crcns-ch-epfl-2009-/

and do mapping between - and _ in argument name for the pipeline function

yarikoptic
yarikoptic reviewed Sep 1, 2016
View reviewed changes
datalad/support/dsconfig.py
# overwrite existing value, do not amend to get multi-line
# values
self._store = _parse_gitconfig_dump(
stdout, self._store, replace=True)
Copy link
Member

@yarikoptic yarikoptic Sep 1, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

so .datalad/config would be overriding any setting -- global or local... kinda suboptimal... I might even say don't we want the reverse -- dataset config (stored in git, might be coming from aliens) to be the first one considered and then possibly augmented by the non-persistent global and/or local?

Copy link
Member

Choose a reason for hiding this comment

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

mih#6

@coveralls
Copy link

coveralls commented Sep 1, 2016 •
edited
Loading

Coverage Status

Coverage increased (+0.2%) to 86.77% when pulling 387b2e2 on mih:nf-dsconfig into 64730e6 on datalad:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 86.77% when pulling 387b2e2 on mih:nf-dsconfig into 64730e6 on datalad:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 86.77% when pulling 387b2e2 on mih:nf-dsconfig into 64730e6 on datalad:master.

@mih mih merged commit 387b2e2 into datalad:master Sep 1, 2016
@mih mih mentioned this pull request Sep 6, 2016
Closed
@mih mih deleted the nf-dsconfig branch June 24, 2017 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@mih @coveralls @codecov-io @yarikoptic