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

Some documentation for _onMetadata #385

Closed
wants to merge 2 commits into from
Closed

Conversation

@OlaviSau
Copy link
Contributor

OlaviSau commented Jul 23, 2015

Writing documentation to understand webtorrent ^^

Writing documentation to understand webtorrent ^^
@OlaviSau

This comment has been minimized.

Copy link
Contributor Author

OlaviSau commented Jul 23, 2015

All suggestions open

@feross

This comment has been minimized.

Copy link
Member

feross commented Jul 24, 2015

Thanks for sending this in. This PR has a few problems:

  1. The tests don't pass. Run npm test to make sure your changes don't break any of the tests.
  2. I prefer succinct inline comments. If you wanted to move some of what you wrote inline, that would be fine.
  3. You also changed some code, so the PR name is misleading. I think this probably introduces a bug.
@feross feross closed this Jul 24, 2015
@OlaviSau

This comment has been minimized.

Copy link
Contributor Author

OlaviSau commented Jul 24, 2015

Thanks for responding :)
Sorry about the tests, pushed it at like 2 am and forgot about them since i had copy pasted from github , appareantly it put some github gibberish into it ^^ last test only required newlines at the end.
About the comments, yeah whatever you like, I wrote it to understand webtorrent better, so perhaps I can contribute in some larger things rather than minor bugfixes :)
And about the code change, the PR is name is misleading indeed sorry about that, but the code change itself is valid, you don't need to check if the object exists if you are checking a property on it.

The following values are always falsy:

false
0 (zero)
"" (empty string)
null
undefined
NaN (a special Number value meaning Not-a-Number!)

Oh and have you considered seperate documentation from code?

@lock lock bot locked as resolved and limited conversation to collaborators May 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.