Page MenuHomeSoftware Heritage

Update docs (getting-started, metadata) with latest changes
ClosedPublic

Authored by ardumont on Fri, May 10, 10:18 AM.

Details

Summary

And associated discovered caveats along the way...

Goals:

  • docs/metadata: Change url & external-id from mandatory to optional
  • docs/getting-started: Update according to latest changes

Fixes:

  • deposit.cli: Fix cli no longer displaying result
  • deposit.client: Make sure the error is propagated to client
  • deposit.cli: Allow back partial deposit

Improvment/Clarification:

  • deposit.cli: Expose a new swh deposit status subcommand
  • deposit.cli.client: Update docstrings
  • cli.client: Improve error message for metadata deposit scenario
  • cli.client: Loosen checks on metadata
  • cli.client: Let click check for filepath existence

Related T1650
Related T1648

Test Plan

tox & docker-dev

Diff Detail

Repository
rDDEP swh-deposit
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

ardumont created this revision.Fri, May 10, 10:18 AM
olasd added a subscriber: olasd.Fri, May 10, 10:23 AM
olasd added inline comments.
docs/getting-started.rst
39 ↗(On Diff #4766)

This should be a reference to an anchor rather than a hardcoded file name.

ardumont added inline comments.Fri, May 10, 10:23 AM
docs/getting-started.rst
247 ↗(On Diff #4766)

I found this quite clearer but also allowed to disentangle a little the mess in the parsing function...

To the expense of some more duplication i intend to remove later (when we factor this client out and i finally take the time to take a look at how to test the cli)...

swh/deposit/cli/__init__.py
23

Without this, this no longer displayed anything...
Which i think you warned me a little earlier during the week ;)

This fixed both swh-deposit and swh deposit which were divergent in behavior.

ardumont added inline comments.Fri, May 10, 10:28 AM
swh/deposit/cli/client.py
339 ↗(On Diff #4766)

I'm wondering if print would not be better here...
As the output is a dict, today, that gives for example:

INFO:swh.deposit.cli.client:{'deposit-id...}

whereas a bare dict outputed (with no noise) would be simpler to use for a client wanted to do something with the json-like output.

I guess there is also the formatter configuration route...

What do you think, could print be ok here?

vlorentz added inline comments.
swh/deposit/cli/__init__.py
23

How can ctx.obj not be None?

ardumont updated this revision to Diff 4768.Fri, May 10, 10:36 AM
  • docs: Use anchor instead of hard-coded filename
vlorentz added inline comments.Fri, May 10, 10:39 AM
swh/deposit/cli/__init__.py
23

nvm, got it. What about doing this instead?

ctx.ensure_object(dict)
log_level = ctx.obj.get('log_level', logging.INFO)
ardumont added inline comments.Fri, May 10, 10:39 AM
swh/deposit/cli/__init__.py
23

Because, from my understanding of the latest refactoring on cli (swh-core), depending on how you call this, this can be populated.

That's what i meant in my first comment here about the behavior divergence between swh deposit (using swh-core inheritance, i'd expect ctx.obj not none here) and (swh-deposit, no init, i'd expect it to be none).

ardumont added inline comments.Fri, May 10, 10:42 AM
swh/deposit/cli/__init__.py
23

I tried the second line initially without and that failed.
Probably because of the missing first line which i did not add because of the swh-core 'inheritance' (<~ probably not the right term)...
This is already done there so i don't know the implication of doing it twice... (for that case).

I'll check

Thanks for the heads up.

ardumont added inline comments.Fri, May 10, 10:44 AM
swh/deposit/cli/__init__.py
23

It works, awesome.
I'll change.

ardumont updated this revision to Diff 4770.Fri, May 10, 10:45 AM
  • deposit.cli: Fix cli no longer displaying result

Better fix suggested by @vlorentz \m/

douardda accepted this revision.Fri, May 10, 11:12 AM

LGTM but I did not do a very punctilious review

This revision is now accepted and ready to land.Fri, May 10, 11:12 AM

Looks great!
small language changes to clarify..

docs/getting-started.rst
247 ↗(On Diff #4766)

this is a great change!!

74 ↗(On Diff #4770)

I would erase the optionally and explain afterwards that you can omit the software name by adding metadata in the file. for example:

software artifact's name (optional if a metadata filepath is specified and the artifact's name is included in the metadata file).
77 ↗(On Diff #4770)

Same as above:

author's name (optional if a metadata filepath is specified  and the authors are included in the metadata file).
This can be specified multiple times in case of multiple authors.
81 ↗(On Diff #4770)

the client uses for the software object. If not provided, A UUID will be generated by SWH.

docs/metadata.rst
37 ↗(On Diff #4770)

erase, we keep only the codemeta option (but I haven't had the time to modify it myself)

39 ↗(On Diff #4770)

keep

41 ↗(On Diff #4770)

erase (this does not mean we change the code, just the way we present what we want

swh/deposit/cli/client.py
137 ↗(On Diff #4770)

needs=> requires

- A multipart deposit (create or update) requires:
         -an existing software compressed archive
         -an existing metadata file or authors and name provided in params
141 ↗(On Diff #4770)

requires

144 ↗(On Diff #4770)

requires

145 ↗(On Diff #4770)

without ( )

metadata file or the authors and software artifact name provided in params
147 ↗(On Diff #4770)

requires
and erase to be provided

A deposit update requires a deposit_id

LGTM but I did not do a very punctilious review

Thanks!

Looks great!
small language changes to clarify..

Awesome, thanks, i will adapt accordingly!

ardumont marked 11 inline comments as done.Fri, May 10, 11:39 AM
ardumont added inline comments.
docs/getting-started.rst
77 ↗(On Diff #4770)

Positive sentence are clearer indeed!

ardumont updated this revision to Diff 4772.Fri, May 10, 11:42 AM
ardumont marked an inline comment as done.

Docs: Improve sentence phrasing

ardumont updated this revision to Diff 4773.Fri, May 10, 11:48 AM

Rework commits and messages

This revision was automatically updated to reflect the committed changes.