Skip to content

Too many parameters bug #554

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

Closed
maasencioh opened this issue Sep 21, 2016 · 3 comments · Fixed by #732
Closed

Too many parameters bug #554

maasencioh opened this issue Sep 21, 2016 · 3 comments · Fixed by #732
Labels
Milestone

Comments

@maasencioh
Copy link

Hello, we have the following code documentation:

/**
 * Global spectra deconvolution
 * @param {Array<Number>} x - Independent variable
 * @param {Array<Number>} yIn - Dependent variable
 * @param {Object} [options] - Options object
 * @param {Object} [options.sgOptions] - Options object for Savitzky-Golay filter. See https://github.com/mljs/savitzky-golay-generalized#options
 * @param {Number} [options.minMaxRatio = 0.00025] - Threshold to determine if a given peak should be considered as a noise
 * @param {Number} [options.broadRatio = 0.00] - If `broadRatio` is higher than 0, then all the peaks which second derivative
 * smaller than `broadRatio * maxAbsSecondDerivative` will be marked with the soft mask equal to true.
 * @param {Number} [options.noiseLevel = 3] - Noise threshold in spectrum units
 * @param {Boolean} [options.maxCriteria = true] - Peaks are local maximum(true) or minimum(false)
 * @param {Boolean} [options.smoothY = true] - Select the peak intensities from a smoothed version of the independent variables
 * @param {Boolean} [options.realTopDetection = false] - Use a quadratic optimizations with the peak and its 3 closest neighbors
 * to determine the true x,y values of the peak?
 * @param {Number} [options.heightFactor = 0] - Factor to multiply the calculated height (usually 2)
 * @param {Boolean} [options.boundaries = false] - Return also the inflection points of the peaks
 * @param {Number} [options.derivativeThreshold = 0] - Filters based on the amplitude of the first derivative
 * @return {Array<Object>}
 */
function gsd(x, yIn, options) {
}

module.exports = gsd;

This is rendering this way:

screenshot from 2016-09-21 16-16-54

As you can see it changes the order of the parameters in the function, the last option it's rendered as a parameter and the yIn goes to the bottom.

The generated JSON seems correct and with less arguments behaves as good as before (remove until maxCriteria and works fine again).

@tmcw
Copy link
Member

tmcw commented Sep 21, 2016

Thanks for the report!

We're sorting parameters based on order in the source, but that sorting wasn't sensitive to nested parameters. 58c6892 should fix this issue by only sorting auto-detected parameters and inserting them just at the point where they're needed, before any nested params.

@maasencioh
Copy link
Author

thanks a lot!

@tmcw
Copy link
Member

tmcw commented Nov 16, 2016

2016-11-16 at 11 44 am

Looks like that introduced a regression and will need a second pass.

@tmcw tmcw added the bug label Apr 12, 2017
@tmcw tmcw added this to the beta20 milestone Apr 12, 2017
tmcw added a commit that referenced this issue Apr 15, 2017
This nesting implementation uses a proper recursive tree algorithm

Fixes #554
tmcw added a commit that referenced this issue Apr 15, 2017
This nesting implementation uses a proper recursive tree algorithm

Fixes #554
tmcw added a commit that referenced this issue Apr 15, 2017
This nesting implementation uses a proper recursive tree algorithm

Fixes #554
tmcw added a commit that referenced this issue Apr 15, 2017
This nesting implementation uses a proper recursive tree algorithm

Fixes #554
tmcw added a commit that referenced this issue Apr 15, 2017
This nesting implementation uses a proper recursive tree algorithm

Fixes #554
tmcw added a commit that referenced this issue Apr 15, 2017
This nesting implementation uses a proper recursive tree algorithm

Fixes #554
tmcw added a commit that referenced this issue Apr 15, 2017
This nesting implementation uses a proper recursive tree algorithm

Fixes #554

BREAKING CHANGE: referencing inferred destructure params without
renaming them, like $0.x, from JSDoc comments will no longer
work. To reference them, instead add a param tag to name the
destructuring param, and then refer to members of that name.

Before:

```js
/**
 * @param {number} $0.x a member of x
 */
function a({ x }) {}
```

After:

```js
/**
 * @param {Object} options
 * @param {number} options.x a member of x
 */
function a({ x }) {}
```
tmcw added a commit that referenced this issue Apr 15, 2017
This nesting implementation uses a proper recursive tree algorithm

Fixes #554

BREAKING CHANGE: referencing inferred destructure params without
renaming them, like $0.x, from JSDoc comments will no longer
work. To reference them, instead add a param tag to name the
destructuring param, and then refer to members of that name.

Before:

```js
/**
 * @param {number} $0.x a member of x
 */
function a({ x }) {}
```

After:

```js
/**
 * @param {Object} options
 * @param {number} options.x a member of x
 */
function a({ x }) {}
```
tmcw added a commit that referenced this issue Apr 15, 2017
This nesting implementation uses a proper recursive tree algorithm

Fixes #554

BREAKING CHANGE: referencing inferred destructure params without
renaming them, like $0.x, from JSDoc comments will no longer
work. To reference them, instead add a param tag to name the
destructuring param, and then refer to members of that name.

Before:

```js
/**
 * @param {number} $0.x a member of x
 */
function a({ x }) {}
```

After:

```js
/**
 * @param {Object} options
 * @param {number} options.x a member of x
 */
function a({ x }) {}
```
tmcw added a commit that referenced this issue Apr 15, 2017
This nesting implementation uses a proper recursive tree algorithm

Fixes #554

BREAKING CHANGE: referencing inferred destructure params without
renaming them, like $0.x, from JSDoc comments will no longer
work. To reference them, instead add a param tag to name the
destructuring param, and then refer to members of that name.

Before:

```js
/**
 * @param {number} $0.x a member of x
 */
function a({ x }) {}
```

After:

```js
/**
 * @param {Object} options
 * @param {number} options.x a member of x
 */
function a({ x }) {}
```
@tmcw tmcw closed this as completed in #732 Apr 21, 2017
tmcw added a commit that referenced this issue Apr 21, 2017
* refactor(nest): Better nesting implementation

This nesting implementation uses a proper recursive tree algorithm

Fixes #554

BREAKING CHANGE: referencing inferred destructure params without
renaming them, like $0.x, from JSDoc comments will no longer
work. To reference them, instead add a param tag to name the
destructuring param, and then refer to members of that name.

Before:

```js
/**
 * @param {number} $0.x a member of x
 */
function a({ x }) {}
```

After:

```js
/**
 * @param {Object} options
 * @param {number} options.x a member of x
 */
function a({ x }) {}
```

* Address review comments

* Reduce testing node requirement back down to 4

* Don't output empty properties, reduce diff noise

* Rearrange and document params

* Simplify param inference, update test fixtures. This is focused around Array destructuring: documenting destructured array elements with indices instead of names, because the names are purely internal details

* Use temporary fork to get through blocker
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants