Update JSDoc comments to add missing parameters #362

whymarrh · 2018-09-26T18:03:01Z

This PR updates a few of the JSDoc comments that were incorrectly listing the arguments of the functions they were documenting.

whymarrh · 2018-09-26T18:06:11Z

As an aside: is there a way to add an extra ESLint rule to standard? From the docs:

If you really want to configure hundreds of ESLint rules individually, you can always use eslint directly with eslint-config-standardd to layer your changes on top. standard-eject can help you migrate from standard to eslint and eslint-config-standard.

Would the team be open to ejecting from standard to add, for example, valid-jsdoc?

holgerd77

Thanks for this first PR, some things to discuss and/or update.

I think moving from standard to eslint directly is not an option regarding the added maintenance work. If there is a simple way just to add one rule or the other to standard I would be eventually open to it, but I guess that just contradicts the original idea of the library?

holgerd77 · 2018-09-27T10:12:11Z

lib/bloom.js

@@ -6,6 +6,7 @@ const byteSize = 256
 * drops leading zeroes from a buffer
 * @function dropLeadingZeroes
 * @param {Buffer} buff
+ * @returns {Buffer} a slice of the buffer starting at the first non-zero entry


holgerd77 · 2018-09-27T10:18:31Z

lib/bloom.js

@@ -32,7 +33,7 @@ var Bloom = module.exports = function (bitvector) {
 /**
 * adds an element to a bit vector of a 64 byte bloom filter
 * @method add
- * @param {Buffer} element
+ * @param e - the element


I am not really getting the change here. This calls dropLeadingZeroes() which requires a Buffer.

Yup, should've kept the type, I'll add that back. The main change is the param name which was incorrect (i.e. s/element/e/).

holgerd77 · 2018-09-27T10:19:13Z

lib/bloom.js

 * @method check
- * @param {Buffer} element
+ * @param e - the element


holgerd77 · 2018-09-27T10:21:14Z

lib/bloom.js

+ * checks if multiple topics are in a bloom
+ * @method multiCheck
+ * @param {Buffer} topics
+ * @returns {boolean} Returns {@code true}


Not sure how much consistency we have with this in the code in general, but we should see that we stay with one convention using capital letter at the explanatory comment ("Returns...") or not.

I had noticed that a majority of the @returns clauses started with "Returns" so I'm following that—I can update the comments for consistency in a separate PR if we want?

Ah, I think that's overblown for now. If you do an update with the other changes that should suffice for now.

holgerd77 · 2018-09-27T10:25:47Z

lib/opFns.js

@@ -799,6 +799,7 @@ function trap (err) {
 /**
 * Subtracts the amount needed for memory usage from `runState.gasLeft`
 * @method subMemUsage
+ * @param runState


We normally have the convention to just use {Object} as a type if this is the most we can get 😄 (see other documentation in the library). If this is not wrong per se (I am not a super-comment-syntax expert) I would like to keep here with this as well.

Yup, I'll add the type in

holgerd77 · 2018-09-27T10:26:30Z

lib/opFns.js

@@ -858,6 +860,7 @@ function memLoad (runState, offset, length) {
 * Stores bytes to memory. If an error occurs a string is instead returned.
 * The function also subtracts the amount of gas need for memory expansion.
 * @method memStore
+ * @param runState


holgerd77 · 2018-09-27T10:26:52Z

lib/opFns.js

@@ -829,6 +830,7 @@ function subMemUsage (runState, offset, length) {
 * a string is instead returned. The function also subtracts the amount of
 * gas need for memory expansion.
 * @method memLoad
+ * @param runState
 * @param {BN} offset where to start reading from
 * @param {BN} length how far to read
 * @return {Buffer|String}


Is it return or returns?

Either is valid, I believe @returns is more prevalent in the codebase from what I see currently

There are some @return -> @returns cases which needs to be updated. The following normally wouldn't be a blocker, but if you update please also remove the - from the @param {Buffer} e - the element to check since this introduces a minor inconsistency in the format.

Otherwise this can be merged.

holgerd77 · 2018-09-27T10:27:55Z

lib/runBlockchain.js

@@ -4,6 +4,7 @@ const async = require('async')
 * processes blocks and adds them to the blockchain
 * @method onBlock
 * @param blockchain
+ * @param {Function} cb the callback


holgerd77 · 2018-09-27T10:30:37Z

lib/runCall.js

@@ -10,6 +10,7 @@ const ERROR = exceptions.ERROR
 * runs a CALL operation
 * @method runCall
 * @param opts
+ * @param cb the callback


This should be at the end of the opts.xxx param values (or again, if this is generally wrong, changed everywhere for consistency).

holgerd77 · 2018-09-27T10:31:16Z

lib/stateManager.js

- * @param {Set} address
- * @param {cb} function
+ * @param {Set} addresses a set of addresses
+ * @param {Function} cb the callback


whymarrh · 2018-09-27T12:18:08Z

I think moving from standard to eslint directly is not an option regarding the added maintenance work. If there is a simple way just to add one rule or the other to standard I would be eventually open to it, but I guess that just contradicts the original idea of the library?

Yeah from what I can tell the standardjs setup is quite specific and, as a result, somewhat inflexible. Not moving to ESLint is fine, just thought I'd bring it up as an option for more static analysis.

holgerd77 · 2018-10-02T11:21:19Z

lib/opFns.js

@@ -829,6 +830,7 @@ function subMemUsage (runState, offset, length) {
 * a string is instead returned. The function also subtracts the amount of
 * gas need for memory expansion.
 * @method memLoad
+ * @param runState
 * @param {BN} offset where to start reading from
 * @param {BN} length how far to read
 * @return {Buffer|String}


There are some @return -> @returns cases which needs to be updated. The following normally wouldn't be a blocker, but if you update please also remove the - from the @param {Buffer} e - the element to check since this introduces a minor inconsistency in the format.

Otherwise this can be merged.

whymarrh · 2018-10-02T13:19:26Z

Thanks for the review, I'll update this today

holgerd77 · 2018-10-05T11:55:38Z

Can this be updated so we can merge? That would be great, PR like this with both a broad scope and low-level changes tend to otherwise cause a disproportionate amount of additional work keeping things up-to-date...

@return

sed -i'' 's/@return /@returns /'

whymarrh · 2018-10-05T14:18:39Z

There are some @return -> @returns cases which needs to be updated. The following normally wouldn't be a blocker, but if you update please also remove the - from the @param {Buffer} e - the element to check since this introduces a minor inconsistency in the format.

Updated

holgerd77

Cool, thanks!

holgerd77 requested changes Sep 27, 2018

View reviewed changes

whymarrh force-pushed the update-jsdoc branch from 38077a1 to 4c793dc Compare September 27, 2018 12:15

holgerd77 requested changes Oct 2, 2018

View reviewed changes

holgerd77 added the PR state: needs changes label Oct 2, 2018

whymarrh force-pushed the update-jsdoc branch from 4c793dc to a9bea1d Compare October 5, 2018 14:15

whymarrh added 3 commits October 5, 2018 11:48

Update JSDoc comments to add missing parameters

dd26a29

Fix formatting of JSDoc comments

6c831ea

Update JSDoc comments to s/@return /@returns /

e4621e7

sed -i'' 's/@return /@returns /'

whymarrh force-pushed the update-jsdoc branch from a9bea1d to e4621e7 Compare October 5, 2018 14:18

holgerd77 approved these changes Oct 5, 2018

View reviewed changes

holgerd77 merged commit 9a3dc04 into ethereumjs:master Oct 5, 2018

whymarrh deleted the update-jsdoc branch October 5, 2018 14:48

evertonfraga added the package: vm label Feb 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update JSDoc comments to add missing parameters #362

Update JSDoc comments to add missing parameters #362

whymarrh commented Sep 26, 2018

whymarrh commented Sep 26, 2018

holgerd77 left a comment

holgerd77 Sep 27, 2018

holgerd77 Sep 27, 2018

whymarrh Sep 27, 2018

holgerd77 Sep 27, 2018

holgerd77 Sep 27, 2018

whymarrh Sep 27, 2018

holgerd77 Sep 27, 2018

holgerd77 Sep 27, 2018

whymarrh Sep 27, 2018

holgerd77 Sep 27, 2018

holgerd77 Sep 27, 2018

whymarrh Sep 27, 2018

holgerd77 Oct 2, 2018

holgerd77 Sep 27, 2018

holgerd77 Sep 27, 2018

whymarrh Sep 27, 2018

holgerd77 Sep 27, 2018

whymarrh commented Sep 27, 2018

holgerd77 Oct 2, 2018

whymarrh commented Oct 2, 2018

holgerd77 commented Oct 5, 2018

whymarrh commented Oct 5, 2018

holgerd77 left a comment

Update JSDoc comments to add missing parameters #362

Update JSDoc comments to add missing parameters #362

Conversation

whymarrh commented Sep 26, 2018

whymarrh commented Sep 26, 2018

holgerd77 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whymarrh commented Sep 27, 2018

Choose a reason for hiding this comment

whymarrh commented Oct 2, 2018

holgerd77 commented Oct 5, 2018

whymarrh commented Oct 5, 2018

holgerd77 left a comment

Choose a reason for hiding this comment