Download Helper added

[coveralls]

Coverage decreased (-0.1%) to 83.249% when pulling 45be3bd03e8d5e573071e2b91996161451a4f18a on khoin:remoteDownload into 3fff8d3948b838b6192d3180243c6e591363aea0 on spatialmodel:master.

[khoin]

Hi Chris,

I've made a few changes from last pull request.

Also, I've tried to wrap some commands around download() in inmaputil/cmd.go, but I noticed that only "EmissionShapeFiles" matches the criteria to have download() wrapped around it. This is because the download() function looks for Shp files.

Is this what you're looking for? Or does download.go has to be changed so that it complies not just with .shp files but other files?

[ctessum]

We want to work for shapefiles and other files. I think it should currently work for non-shapefiles that are not compressed. I added a comment about how I think we can make it work files that are compressed but not shapefiles.

Also, there tests are failing because the download function also needs a channel as an argument: https://travis-ci.org/spatialmodel/inmap/jobs/355650570#L486

[khoin]

I've made some changes. It passes 2/3 tests. Pass in Go 1.9.x but not 1.10.x. I will look into that. Still, there are some things you can look over and check if they're all right.

Particularly, I've added a helper function to create a chan that will output to whatever cobra.Command in the scope. I've also noticed that EmissionShapeFiles is an array and not a single file path, so I've wrapped download around each element. Let me know if that looks okay.

Additionally, here are some config variables which are defined, but isn't used anywhere, so I couldn't find and wrap download around them: VarGrid.CensusFile VarGrid.MortalityRateFile

[khoin]

I've changed the code a little bit and now it passes all the tests in Travis CI. It was simply a logging change and I have no clue why it's passing on Travis now.

However, it seems like some files in testdata were changed when I ran the tests and push the commit. I couldn't notice the files added/changed until I commit. I tried to download the repo ZIP and put the files back again then commit again but it still shows up in the Files Changed tab. Do you have any idea how this could be mitigated?

Otherwise, just let me know if download.go, download_test.go, cmd.go is good and I'll just clone again and replace the files.

[ctessum]

I think git has revert and reset commands that can undo changes, but I don't remember the specifics. Google would probably know.

[khoin]

I've made the changes, and cleaned my PR along with having some changes from gopkg.lock from dep ensure.

When I tested it locally and it passed, but Travis CI keeps failing for 1.10. I've also updated Golang on my local machine to 1.10 to test it out but it still passes locally here but not Travis. The Travis log isn't really helpful so I'll look into it later.

Can you see if it also works on your machine?

[khoin]

I've removed the output files from this change so that they're not tracked.

Additionally, I've fixed the test files archive to include other files necessary for inmap to run.

I've also added a pause after the server is initiated (download_test.go:34) but Travis CI still fails the test (for the older Go version). At first, it showed up an error that the port is in use, so I've changed the port number to 12999 for testing now because it seems less likely now. (7777 seems like a nice number people people would use). It still fails for Go 1.9.x nonetheless (with the same invalid memory address error).

[ctessum]

Hi Khoi,

A couple of comments:

• At least one of your errors is caused by passing a nil argument for the context in srv.Shutdown. It should instead be srv.Shutdown(context.Background()). After I change that, the tests run for me without panicking. It probably doesn't need a pause after the server initiation.
• You'll want to use the go auto-formatter to format your code before committing it. There are a number of editor plugins that do this automatically (e.g., goclipse), or you can do it with the gofmt command.
• The port-in-use error is probably caused by running two instances of the tests at once, perhaps in two different terminals or one in an editor and one in the terminal.
• For configExampleRemote.toml, we probably want to have it try downloading all of the input files rather than just the emissions files. I think for the most part this will only require changes in the config file, but we will also need to create zip files for the population and mortality rate shapefiles.
• Change testEmis.shp.zip to testEmis.zip.
• In all cases, for documentation comments, use full sentences with Capitalized first letters and periods. (I understand that not all comments currently follow that rule, but we're trying to improve over time).

[khoin]

Hi Chris,

• I've passed the context for the Shutdown method; it is without errors now. Thanks!
• I've installed gofmt as a plugin for Sublime Text. gofmt is now invoked every time I save the file. The changes are reflected in this commit.
• I haven't encountered the port-clashing error anymore so I haven't changed anything according to that.
• testEmis.shp.zip is renamed.

I've double checked the comments and have removed some that seems unnecessary. The existing ones are with the guidelines. Let me know if something needs to be changed. The exception I made to capitalizing the first letters is when the first word of the sentence is a variable/function name that is not capitalized.

I will try and package the mortality and population rates into zip files and test it out later today. If there's a specific way you want me to package these files (e.g., separately, together, etc.), let me know.

[khoin]

Hi Chris,

I've applied these changes, but I can not find anywhere in cmd.go to wrap download() around to load testPopulation.zip correctly.

I suspect that although the field VarGrid.CensusFile is defined in Cfg variable in cmd.go, it is actually read through VarGridConfig.loadPopulation() here.

A solution is to wrap download() around config.CensusFile in that line linked above, but that means I would also have to modify vargrid.go. Let me know if you approve of this.

The same situation is for MortalityRateFile as well.

[ctessum]

Hi Khoi,

I think you can wrap the download function around the lines here and here. Let me know if that doesn't work.

[khoin]

Hi Chris,

The changes are reflected.

[ctessum]

Fixes #32.

[ctessum]

Thanks! One more comment: can you change the name of the "download" function to "maybeDownload", so that it reflects the fact that it's only downloading files if they're not local?

Then, feel free to merge it.

[khoin]

All renamed now and pushed. Can't merge since I don't have write access.

[ctessum]

I merged it. I don't know why this didn't close automatically.