search

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

[WIP] Overhaul Metadata for individual components #46

Closed SebastinSanty closed 6 years ago

SebastinSanty commented 6 years ago

Please check and let me know what you think. I'll change across other Datarepos ones I get your reviews. Obviously the tests will fail.

oxinabox commented 6 years ago

I am excited, I'll get reviewing this right away

oxinabox commented 6 years ago

I am finished my reviews Looking really good. I think this will be a big improvement for maintainability.

I also like that you are using more single line functions (when the lines are short enough)

oxinabox commented 6 years ago

Getting there.

As some point some things became camelCased

Like

Can they be changed to snake_case

SebastinSanty commented 6 years ago

Sure, I'll switch. Personally I prefer camelcase that's why it came in.

On Wed, 11 Jul 2018, 05:19 Lyndon White, notifications@github.com wrote:

Getting there.

As some point some things became camelCased

Like

  • publishedDate
  • createDate
  • modifiedDate
  • paperCite
  • datasetCite

Can they be changed to snake_case

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/oxinabox/DataDepsGenerators.jl/pull/46#issuecomment-404102834, or mute the thread https://github.com/notifications/unsubscribe-auth/ANKBzHi5sSs8HvLVz1zM7F1kMBECJPVEks5uFcMKgaJpZM4VI3-J .

oxinabox commented 6 years ago

Sure, I'll switch. Personally I prefer camelcase that's why it came in.

I've stopped having personal preference for casing; since I feel consistency within project and with other projects matters more. (I even give up on my more strongly held preference for tabs over spaces :-o)

codecov-io commented 6 years ago

Codecov Report

:exclamation: No coverage uploaded for pull request base (master@b2e5bc4). Click here to learn what that means. The diff coverage is 94.56%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #46   +/-   ##
=========================================
  Coverage          ?   93.22%           
=========================================
  Files             ?       18           
  Lines             ?      369           
  Branches          ?        0           
=========================================
  Hits              ?      344           
  Misses            ?       25           
  Partials          ?        0
Impacted Files Coverage Δ
src/DataOneV2/KNB.jl 85.71% <0%> (ø)
src/DataOneV2/ArcticDataCenter.jl 100% <100%> (ø)
src/DataOneV1.jl 100% <100%> (ø)
src/DataCite.jl 95.65% <100%> (ø)
src/Figshare.jl 96.29% <100%> (ø)
src/DataOneV2/KnowledgeNetworkforBiocomplexity.jl 100% <100%> (ø)
src/DataOneV2/TERN.jl 100% <100%> (ø)
src/JSONLD/JSONLD.jl 96.42% <100%> (ø)
src/DataOneV2/DataOneV2.jl 53.84% <100%> (ø)
src/CKAN.jl 94.11% <100%> (ø)
... and 3 more

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 b2e5bc4...890cded. Read the comment docs.

oxinabox commented 6 years ago

I have noticed the indenting of the final output is all screwed up. It needs to be:

regisister(DataDep(
    "NAME",
    """
    MSG1
    MSG2
    """,
   ["URL1", "URL2]
    "CHECKSUM"
))

Every line on the inside needs to be indented. At the start of each line.

Take a look at the string at the end of the current current structure: https://github.com/oxinabox/DataDepsGenerators.jl/blob/b2e5bc460930dcde4656ab750487b5188e8741f0/src/DataDepsGenerators.jl#L62-L81 and note that it does include indents

Not that the indent function exits for indenting multiline strings. It splits the string into each line and then appends a tab to each, before joining it back into one string. So it was handling the multiline string message ware returning.

Related: I think that message function should be only generating the free-text message (which is the name of that field in the DataDep structure). Rather than the whole thing.

I think this is all generally on the right track, can you go through every concern I raised (you'll have to click "expand outdated in github"), and I will do a code review once everything is done.

SebastinSanty commented 6 years ago

Pinging on the PR

oxinabox commented 6 years ago

The Dataset fullname is still missing from the message text. It is need there.l Other than that looks mostly good. I've not checked over my full list of issues I raised with earlier ones. So hopefully you have.

I am pretty hugely busy today, but I will merge it once that missing dataset name in messages is fixed, assuming you are happy with it.

Other changes can come in other PRs.

oxinabox commented 6 years ago

🎉