Skip to content

Conversation

meyer1994
Copy link
Contributor

Changelog

  • Added the dag attribute to client
  • The dag attribute has the following methods:
    • put
    • get
    • resolve
    • import
    • export
  • Added tests
  • Added docs

Notes

This implementation was basically copying and pasting from other parts of the codebase. The tests were created in the same fashion. 😅

@meyer1994
Copy link
Contributor Author

meyer1994 commented Aug 20, 2020

About the error in Travis, it seems that version 0.4.22 does not support ipfs dag import/export. By looking at the changelog, it seems that support for it was added in v.0.5.0. Maybe change the compat verion to a newer version? I am very new on this project. I do not know if it will break something or not.

Copy link
Contributor

@ntninja ntninja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Just found some minor stuff and the whitespace issue that I'll take care of.

Glad to have this implemented!

Comment on lines 25 to +26
#TODO: `from . import dag`
from . import dag
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You forgot to remove the TODO comment above 😉

Comment on lines +44 to +45
maximum: str = VERSION_MAXIMUM,
blacklist: ty.Iterable[str] = VERSION_BLACKLIST) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The file got littered with some problematic whitespace changes unfortunately. I'll update your commit to remove this part of the diff, please don't be offended.

Which program did you use to edit the file?

Comment on lines +103 to +104
def imprt(self, data: ty.IO, **kwargs: base.CommonArgs):
"""Imports a .car file with a DAG to IPFS
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should include a comment about why the name is spelled incorrectly (it wasn't immediately obvious to me and I'm sure there are lots of less experienced Python programmers that wouldn't get it all and just be annoyed by what they think is a typo).

class Section(base.SectionBase):
@base.returns_single_item(base.ResponseBase)
def get(self, cid: str, **kwargs: base.CommonArgs):
"""Get and serialize the DAG node named by CID.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Tone of voice is different here from other other doc-strings.

@ntninja ntninja mentioned this pull request Aug 24, 2020
@ntninja
Copy link
Contributor

ntninja commented Aug 24, 2020

Merged in #232. Thank you!

@ntninja ntninja closed this Aug 24, 2020
@ntninja
Copy link
Contributor

ntninja commented Aug 26, 2020

@meyer1994: I just realized that this was missing support for the optional parameters of /dag/put. Could you add those?

It would allow importing directories based on lists of CIDs like requested in #220. 🙂

@meyer1994 meyer1994 deleted the dag branch September 2, 2020 12:35
@meyer1994
Copy link
Contributor Author

@ntninja I will try to do it this weekend :)

@ntninja
Copy link
Contributor

ntninja commented Sep 2, 2020

Thanks! 🙂

@meyer1994
Copy link
Contributor Author

Created PR #238

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants