Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP Modernize documentation and examples #205

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Conversation

djdv
Copy link
Contributor

@djdv djdv commented Feb 9, 2021

I keep referencing these docs which are several years out of date or missing. I'm going to start tracking my findings instead of forgetting them, but probably won't make a complete pass.
This is really rough right now but when more is done, I'll flip this from a draft.
(Not sure when I'll get to it but I'm going to be reviewing code that deals with this library soon)

In the meantime, anyone feel free to point things out here.
X is missing, Y is wrong, your update of Y is wrong, etc.

Commit history is going to be force pushed over randomly, so don't depend on it until we're out of draft at least.

  • every docstring needs a pass, empty ones should be filled in
  • I need to rewrite most of the new example, but it's a base

@djdv
Copy link
Contributor Author

djdv commented Feb 10, 2021

Reminders:

  • There should probably be an explicit remark around either ReqLog and/or the http.NewHandler, which informs the user of the relation between them.
    • also consider defining a public version of this interface rather than using an adhoc one in http
    • The opaqueness around env in the handler constructor should be made clear, the user should probably be informed that it's going to be passed to Call; as-is there's no remark, just pass in an empty interface{} and ???.
  • No harm in expressing that this provides a blank config rather than a config with default (non-zero) values; as-is no remark/not clear
  • docs defined but not attached
    • put it in the preamble and/or copy-paste for now, later maybe go generate tags
  • this error message
    • value was defined when these files where still within the go-ipfs repo. It doesn't make sense for cmds/http servers who are not go-ipfs.
    • a more generic message with an exported error variable would be nice. Callers can then just check for it via errors.Is and add program specific context at the callsite. e.g. go-ipfs checks for something like ErrConnectAPI, and appends its help message about $IPFS_PATH itself.

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.

1 participant