Skip to content

Conversation

@mgxd
Copy link
Contributor

@mgxd mgxd commented Apr 13, 2020

closes #44

@codecov
Copy link

codecov bot commented Apr 13, 2020

Codecov Report

Merging #45 into master will increase coverage by 0.28%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #45      +/-   ##
==========================================
+ Coverage   88.05%   88.34%   +0.28%     
==========================================
  Files           5        5              
  Lines         201      206       +5     
==========================================
+ Hits          177      182       +5     
  Misses         24       24              
Flag Coverage Δ
#api 88.34% <100.00%> (+0.28%) ⬆️
#config ?
#datalad 78.64% <92.30%> (+0.03%) ⬆️
#dls3 55.82% <76.92%> (+14.03%) ⬆️
#s3 59.22% <76.92%> (+4.99%) ⬆️
Impacted Files Coverage Δ
templateflow/__init__.py 100.00% <100.00%> (ø)
templateflow/conf/__init__.py 91.30% <100.00%> (ø)
templateflow/conf/_s3.py 97.91% <100.00%> (+0.09%) ⬆️

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 c1ee264...ea7dd5c. Read the comment docs.

@mgxd mgxd force-pushed the enh/auto-update branch from e84d0ec to ae0d510 Compare April 13, 2020 17:13
@mgxd mgxd force-pushed the enh/auto-update branch from ae0d510 to 5329ae8 Compare April 13, 2020 17:17
@mgxd mgxd marked this pull request as ready for review April 21, 2020 20:05
@mgxd mgxd force-pushed the enh/auto-update branch from bf0e5a8 to 9a31a6d Compare April 21, 2020 20:08
@mgxd mgxd force-pushed the enh/auto-update branch from 9a31a6d to 67e7ffc Compare April 21, 2020 20:12
@oesteban oesteban changed the title ENH: automate skeleton update on import ENH: Run an automatic skeleton update on import by default Apr 24, 2020
Copy link
Member

@oesteban oesteban left a comment

Choose a reason for hiding this comment

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

Looking good - I have added two suggestions to avoid this eager update when DataLad is the backend. (after all, that's for power users so those should know how to update themselves - and still can call update() explicitly). Please check it works as expected.

Also, for other PR, some tests would be nice.

def update(local=False, overwrite=True):
def update(local=False, overwrite=True, silent=False):
"""Update an existing DataLad or S3 home."""
if TF_USE_DATALAD and _update_datalad():
Copy link
Member

Choose a reason for hiding this comment

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

have you checked this update thing with the datalad operation mode? - possibly for a further PR of refinements of that use-case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope - though just did and:

Updating TEMPLATEFLOW_HOME using DataLad ...
[ERROR  ] path not associated with any dataset [update(/Users/mathiasg/.cache/templateflow)] 
/Users/mathiasg/code/templateflow/templateflow/conf/__init__.py:74: UserWarning: Error updating TemplateFlow's home directory (using DataLad): Command did not complete successfully [{'action': 'update', 'path': '/Users/mathiasg/.cache/templateflow', 'type': 'directory', 'raw_input': True, 'orig_request': '/Users/mathiasg/.cache/templateflow', 'status': 'error', 'message': 'path not associated with any dataset'}]
  warn(f"Error updating TemplateFlow's home directory (using DataLad): {e}")

so I'll commit your changes to avoid the auto-update if the user prefers DataLad.

@oesteban
Copy link
Member

Bumping this up, any updates?

Co-Authored-By: Oscar Esteban <code@oscaresteban.es>
@oesteban
Copy link
Member

This is ready, correct?

@mgxd
Copy link
Contributor Author

mgxd commented Apr 30, 2020

yes - update is still disabled by default when using datalad. do we want to revert that now that #48 is in?

@oesteban
Copy link
Member

Nope, datalad users are supposed to be knowledgeable enough to call update() themselves 👍 . Merging!! thanks for this :)

@oesteban oesteban merged commit 6a024f4 into templateflow:master Apr 30, 2020
@oesteban
Copy link
Member

With this merged, we should cut a new release, DYT?

@mgxd mgxd deleted the enh/auto-update branch April 30, 2020 21:25
@oesteban
Copy link
Member

I'll go for it

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.

Add update() call when importing templateflow

2 participants