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

Data directory should not be created on import #2019

Merged
merged 8 commits into from
Mar 27, 2017
Merged

Data directory should not be created on import #2019

merged 8 commits into from
Mar 27, 2017

Conversation

nitinkgp23
Copy link
Contributor

@nitinkgp23 nitinkgp23 commented Mar 6, 2017

Fixes #2018

Before:

Upon doing import sunpy, a directory was created on /home/sunpy/data , irrespective of whether we download data or not.

After:

Upon doing import sunpy, the path is saved in the config file but the directory is not created. The directory is first created when we try downloading any data.

@pep8speaks
Copy link

pep8speaks commented Mar 6, 2017

Hello @nitinkgp23! Thanks for updating the PR.

Line 504:1: E302 expected 2 blank lines, found 1
Line 511:1: E302 expected 2 blank lines, found 1
Line 820:101: E501 line too long (110 > 100 characters)
Line 821:101: E501 line too long (109 > 100 characters)
Line 852:68: E251 unexpected spaces around keyword / parameter equals
Line 852:70: E251 unexpected spaces around keyword / parameter equals
Line 853:67: E251 unexpected spaces around keyword / parameter equals
Line 853:69: E251 unexpected spaces around keyword / parameter equals

Line 977:5: E122 continuation line missing indentation or outdented

Line 126:16: E251 unexpected spaces around keyword / parameter equals
Line 126:18: E251 unexpected spaces around keyword / parameter equals

Line 173:17: E129 visually indented line with same indent as next logical line

Line 4:1: E265 block comment should start with '# '
Line 42:27: E261 at least two spaces before inline comment
Line 42:28: E262 inline comment should start with '# '
Line 44:5: E306 expected 1 blank line before a nested definition, found 0
Line 65:1: E302 expected 2 blank lines, found 1
Line 79:1: E302 expected 2 blank lines, found 1
Line 96:5: E731 do not assign a lambda expression, use a def
Line 103:101: E501 line too long (103 > 100 characters)
Line 104:101: E501 line too long (101 > 100 characters)
Line 117:1: E302 expected 2 blank lines, found 1
Line 126:9: E731 do not assign a lambda expression, use a def
Line 148:1: E302 expected 2 blank lines, found 1
Line 153:5: E731 do not assign a lambda expression, use a def

Line 100:33: E122 continuation line missing indentation or outdented

Comment last updated on March 21, 2017 at 16:04 Hours UTC

@nitinkgp23
Copy link
Contributor Author

Hey @Cadair, Can you please review the PR?

@Cadair
Copy link
Member

Cadair commented Mar 7, 2017

Thanks for the PR!

It's not only the sample data that uses the sunpy/data directory. It is the default location for the VSO and other clients in sunpy.net. I think a helper in the config module for "get or create download dir" and a through search through the code for instances of the download dir.

@nitinkgp23
Copy link
Contributor Author

Yeah, I thought so. But, I checked it by directly downloading a file using jsoc client, after deleting the download directory at $HOME and the directory got automatically created and download was successful.

@Cadair
Copy link
Member

Cadair commented Mar 7, 2017

It seems that some of the travis errors here are due to the directory not existing: https://travis-ci.org/sunpy/sunpy/jobs/208183122#L2488

Seems that it's in lightcurve?

@nitinkgp23
Copy link
Contributor Author

nitinkgp23 commented Mar 7, 2017

Ahh :( . So what's the best way out? Make a through search, and add the command of creating directory everywhere? Or, is there any shortcut, that I am missing?

@Cadair
Copy link
Member

Cadair commented Mar 7, 2017

I would make a function in config which gets the config value of the download dir, and makes the dir if it does not exist. Then search through the code and replace the download dir get call with that function.

@nitinkgp23
Copy link
Contributor Author

Okkay, I will make the changes :). Thanks.

@@ -88,6 +89,7 @@
GOES_REMOTE_PATH = "http://{0}/ssw/gen/idl/synoptic/goes/".format(HOST)
# Define location where data files should be downloaded to.
DATA_PATH = config.get("downloads", "download_dir")
create_download_dir()
Copy link
Member

Choose a reason for hiding this comment

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

There is no need for two function calls here (get default directory and create it) this can be done in one call.

Also, this is still to early to create the directory. We only want to be creating it upon use, so if the functions are called with the default data dir we should make the directory then, rather than on import of this submodule.

To give you an example for this file you want to replace every occurrence of DATA_PATH with sunpy.config.get_and_create_download_dir().

Copy link
Member

Choose a reason for hiding this comment

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

This is the same for the other files as well.

Copy link
Contributor Author

@nitinkgp23 nitinkgp23 left a comment

Choose a reason for hiding this comment

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

@Cadair I will modify other files too, if this is okay.

@@ -377,7 +374,7 @@ def _goes_chianti_tem(longflux, shortflux, satellite=8,

"""
if not download_dir:
download_dir = DATA_PATH
download_dir = get_and_create_download_dir()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this what we want?

Copy link
Member

Choose a reason for hiding this comment

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

looks good!

@@ -76,6 +76,10 @@ def download_sample_data(progress=True, overwrite=True, timeout=None):
-------
None
"""
# Creating the directory for sample files to be downloaded
Copy link
Member

Choose a reason for hiding this comment

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

This can use the function.

@Cadair
Copy link
Member

Cadair commented Mar 15, 2017

also @nitinkgp23 if you merge master in or rebase the build should pass.

@Cadair Cadair added 0.7.x Affects Release An issue/bug that affects a released version (use a version label too) labels Mar 15, 2017
@Cadair
Copy link
Member

Cadair commented Mar 15, 2017

@nabobalis Your first backport.

@Cadair
Copy link
Member

Cadair commented Mar 22, 2017

@kbj Will this fix your concerns?

@Cadair Cadair merged commit 52544da into sunpy:master Mar 27, 2017
@nitinkgp23 nitinkgp23 deleted the issue2018 branch March 27, 2017 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Affects Release An issue/bug that affects a released version (use a version label too)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants