oxinabox / DataDepsGenerators.jl

Utility for developers to help define DataDeps registration blocks, for reusing existing Data with DataDeps.jl
Other
18 stars 6 forks source link

Reword the APIs #27

Closed SebastinSanty closed 6 years ago

SebastinSanty commented 6 years ago

I propose this rewording for the following reasons (which I have added as comments also):

Please let me know what you think of.

oxinabox commented 6 years ago

The notion seems right, looks like some of the reference tests need to be updated. Once you have them passing feel free to merge

SebastinSanty commented 6 years ago

The tests are running fine locally :(

oxinabox commented 6 years ago

They should not be passing locally, the reference file for Model forage fish supply (ecological supply) is wrong and the actual file is right.

According to http://dx.doi.org/doi:10.5063/F1T43R7N and the actual file there should be 33 files to download. But the reference file lists only 15 of them.

It might be that it was updated. It might be good to try and select fairly old datasets for testing as they are less likely to be updated.

SebastinSanty commented 6 years ago

No clue why this is happening to me. But tests are passing locally. Even the text which is getting downloaded is matching with the reference.

Can you try running it locally?

oxinabox commented 6 years ago

Everything (except github) is passing for me too

oxinabox commented 6 years ago

Right, We are hitting https://github.com/JuliaWeb/Gumbo.jl/issues/46

Which is fixed in the newish Gumbo v0.4.1 (which is used by CI), but is present in v0.4.0 which I (and I suspect you) have locally.

Check what the output of copy pasting the following into the REPL is:

using DataDepsGenerators
const DDG = DataDepsGenerators
using AbstractTrees, Gumbo
page = DDG.getpage("https://knb.ecoinformatics.org/knb/d1/mn/v2/object/doi:10.5063/F1T43R7N");
target_sec = page.root[2][1][2][23]
[elem for elem in StatelessBFS(target_sec)]

It should include the text:

 Gumbo.HTMLElement{:url}:
<url function="download">
  ecogrid://knb/msleckman.51.1
</url>

and not something like:

Gumbo.HTMLElement{Symbol("url function=\"123232\"")}

So the solution is to run Pkg.update() locally, regenerate the tests, and update the REQUIRE file to demand Gumbo 0.4.1

SebastinSanty commented 6 years ago

Funny. So I had updated the packages to check this issue. It stopped at DataDepsGenerators not being able to update. Considering it normal, I just moved on. But little did I realize that not all packages were updated.

SebastinSanty commented 6 years ago

Need to merge #33 before this.