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

Partly resolved Issue 75 #82

Merged
merged 14 commits into from
Feb 25, 2020
Merged

Partly resolved Issue 75 #82

merged 14 commits into from
Feb 25, 2020

Conversation

NateMietk
Copy link
Contributor

The main problem was download was hanging (#75), but that was not on the neonUtilities side of things. A cache refresh solved the problem on its own, but added while loop checks, similar to #69, and this should solve any potential for future infinite while loops if a url is broken or there are no data found.

@NateMietk
remove default site and year. instead added error messages if those a…
d6534b0 
…re not defined.
@NateMietk
moved getFileUrls to standalone script
e560af4
@NateMietk
added counters inthe while loop to eliminate the possibility of an in…
95e8e88 
…finite a loop. The while loop will now exit after 2 tries - any more than that the error is not because of an expired url.
@NateMietk NateMietk requested a review from cklunch February 12, 2020 22:19
@cklunch
Copy link
Collaborator

cklunch commented Feb 13, 2020

@NateMietk
It looks like this pull request contains the changes for both the byFileAOP() loop fix and the saveUnzippedFiles behavior. Can you remove the saveUnzippedFiles changes from this branch to keep things simple?

For the byFileAOP() changes, same comments as in byTileAOP(). Thanks!

@NateMietk
Copy link
Contributor Author

@cklunch updated and push the original cleanUp function

@cklunch
Copy link
Collaborator

cklunch commented Feb 14, 2020

@NateMietk
We shouldn't need the tileEasting and tileNorthing parameters here, since byFileAOP() doesn't allow subset by tile.

PR review now says there's a merge conflict on cleanUp.R, can you check it out?

@NateMietk
Copy link
Contributor Author

@cklunch That's right - sorry about that.

@NateMietk
Copy link
Contributor Author

@cklunch I believe this issue is now resolved. testUtilities comes up with no error and the files download without a problem.

@cklunch
Copy link
Collaborator

cklunch commented Feb 25, 2020

Looking good. Verified url regeneration is still happening, and updated doc files look fine.

@cklunch cklunch merged commit 0c313cd into NEONScience:master Feb 25, 2020
@NateMietk NateMietk deleted the issue-75 branch March 4, 2020 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cklunch cklunch Awaiting requested review from cklunch

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@NateMietk @cklunch