Skip to content
This repository was archived by the owner on Dec 4, 2018. It is now read-only.

Parse array default syntax #132

Merged
merged 1 commit into from
Oct 30, 2015
Merged

Parse array default syntax #132

merged 1 commit into from
Oct 30, 2015

Conversation

tmcw
Copy link
Contributor

@tmcw tmcw commented Oct 28, 2015

Fixes #133

this allows the code

@param {Array} [foo=[1]] to properly be parsed

It does this by keeping track of bracket depth in the name parsing.

@nzakas
Copy link
Member

nzakas commented Oct 28, 2015

Thanks for the PR. We're now following the conventions used by the rest of the ESLint organization, so please take a moment to read over how to format your PR: http://eslint.org/docs/developer-guide/contributing/pull-requests

Can you also file an issue describing the problem and point to any documentation showing that this pattern should work? Thanks!

@tmcw
Copy link
Contributor Author

tmcw commented Oct 28, 2015

Currently I can't run npm test because it fails with

> gulp

[17:23:32] Using gulpfile ~/src/doctrine/gulpfile.js
[17:23:32] Starting 'lint'...
[17:23:32] Starting 'test'...

/Users/tmcw/src/doctrine/node_modules/eslint/lib/eslint.js:344
            if (environments[env].ecmaFeatures) {
                                 ^
TypeError: Cannot read property 'ecmaFeatures' of undefined
    at /Users/tmcw/src/doctrine/node_modules/eslint/lib/eslint.js:344:34
    at Array.forEach (native)
    at prepareConfig (/Users/tmcw/src/doctrine/node_modules/eslint/lib/eslint.js:343:33)
    at EventEmitter.module.exports.api.verify (/Users/tmcw/src/doctrine/node_modules/eslint/lib/eslint.js:568:18)
    at verify (/Users/tmcw/src/doctrine/node_modules/gulp-eslint/index.js:21:25)
    at DestroyableTransform._transform (/Users/tmcw/src/doctrine/node_modules/gulp-eslint/index.js:44:18)
    at DestroyableTransform.Transform._read (/Users/tmcw/src/doctrine/node_modules/gulp-eslint/node_modules/readable-stream/lib/_stream_transform.js:184:10)
    at DestroyableTransform.Transform._write (/Users/tmcw/src/doctrine/node_modules/gulp-eslint/node_modules/readable-stream/lib/_stream_transform.js:172:12)
    at doWrite (/Users/tmcw/src/doctrine/node_modules/gulp-eslint/node_modules/readable-stream/lib/_stream_writable.js:237:10)
    at writeOrBuffer (/Users/tmcw/src/doctrine/node_modules/gulp-eslint/node_modules/readable-stream/lib/_stream_writable.js:227:5)

@nzakas
Copy link
Member

nzakas commented Oct 28, 2015

Can you try running npm install and then npm test again?

@tmcw
Copy link
Contributor Author

tmcw commented Oct 28, 2015

Re: docs, the docs for param show an example of a default value and indicate no other way that a default value could be specified. The usejsdoc docs tend to be very light on specificity, so I'd err on supporting a wider variety of values here.

@tmcw
Copy link
Contributor Author

tmcw commented Oct 28, 2015

Removing and reinstalling all modules produces the same result: https://gist.github.com/tmcw/e5c9edee3e55f34464bf

@nzakas
Copy link
Member

nzakas commented Oct 28, 2015

Can you verify with JSDoc itself that the syntax works? My concern is allowing something that JSDoc will throw an error on. I want to be lenient, but not the point of breaking compatibility.

Hmm, strange. Looks like Travis is okay and I can run it fine locally as well. Are you using npm 3?

@tmcw
Copy link
Contributor Author

tmcw commented Oct 28, 2015

Yes, using [email protected].

I can confirm jsdoc 3.3.3 parses this syntax:

2015-10-28 at 5 37 pm

From the comment:

/**
 * Exclude given access levels from the generated documentation: this allows
 * users to write documentation for non-public members by using the
 * `@private` tag.
 *
 * @param {string} firstName - The user's first name.
 * @param {Array<string>} [levels=['private']] excluded access levels.
 * @param {Array<Object>} comments parsed comments (can be nested)
 * @return {Array<Object>} filtered comments
 */

@nzakas
Copy link
Member

nzakas commented Oct 28, 2015

Cool, can you add that information to an issue? We're going to show the changelog with each release and like to link to issues.

There are some issues using ESLint with npm 3, something to do with the flattening of dependencies. Travis passes, so we should be all good.

this allows the code

    @param {Array} [foo=[1]] to properly be parsed

It does this by keeping track of bracket depth in the name parsing.
@tmcw
Copy link
Contributor Author

tmcw commented Oct 28, 2015

Added a ticket and amended the commit to mention it.

@tmcw
Copy link
Contributor Author

tmcw commented Oct 30, 2015

(also signed the CLA)

@nzakas
Copy link
Member

nzakas commented Oct 30, 2015

Looks good. Thanks!

nzakas added a commit that referenced this pull request Oct 30, 2015
@nzakas nzakas merged commit 4a5823b into eslint:master Oct 30, 2015
@tmcw tmcw deleted the parse-default-array branch October 30, 2015 17:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants