Skip to content
This repository has been archived by the owner on Mar 23, 2024. It is now read-only.

disallowIdentifierNames: fix errors with using object properties as a id... #1204

Closed
wants to merge 1 commit into from

Conversation

hzoo
Copy link
Member

@hzoo hzoo commented Mar 27, 2015

...entifier

Fixes #1203

@mrjoelkemp
Copy link
Member

Lgtm

@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.09% when pulling 975300a on hzoo:disallowIdentifierNames-fix into c668ff1 on jscs-dev:master.

@hzoo hzoo force-pushed the disallowIdentifierNames-fix branch from 975300a to 6b2c360 Compare March 27, 2015 21:15
@hzoo
Copy link
Member Author

hzoo commented Mar 27, 2015

Changed to check with hasOwnProperty.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.09% when pulling 6b2c360 on hzoo:disallowIdentifierNames-fix into c668ff1 on jscs-dev:master.

@elicwhite
Copy link
Contributor

LGTM

@@ -54,14 +54,14 @@ module.exports.prototype = {
var disallowedIdentifiers = this._identifierIndex;

file.iterateNodesByType('Identifier', function(node) {
if (disallowedIdentifiers[node.name]) {
if (disallowedIdentifiers.hasOwnProperty(node.name)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually this will cause a problem if a hasOwnProperty key exists on the object. You want Object.prototype.hasOwnProperty.call(disallowedIdentifiers, node.name)

@ljharb
Copy link
Contributor

ljharb commented Apr 1, 2015

@hzoo any chance of fixing my comment? I'd love to see this merged and released asap :-)

@hzoo hzoo force-pushed the disallowIdentifierNames-fix branch from 6b2c360 to 3bd61b3 Compare April 1, 2015 12:35
@hzoo
Copy link
Member Author

hzoo commented Apr 1, 2015

@ljharb Made the change. Should I write a test for this? (configure with ["hasOwnProperty"])

@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.09% when pulling 3bd61b3 on hzoo:disallowIdentifierNames-fix into c668ff1 on jscs-dev:master.

@ljharb
Copy link
Contributor

ljharb commented Apr 1, 2015

@hzoo that's up to @mrjoelkemp and the collaborators. I don't typically write a test for this case, because one would also have to test for propertyIsEnumerable, constructor, and a number of others, but I don't think anyone would complain if you had a test covering it.

@mrjoelkemp any chance this could be merged and published soon? :-)

@mikesherov
Copy link
Contributor

@ljharb yeah, I can land some PR's and release a patch version tonight.

@ljharb
Copy link
Contributor

ljharb commented Apr 1, 2015

@mikesherov hooray, thanks! I've got lots of repos I want to bump jscs on

@mikesherov
Copy link
Contributor

@ljharb got delayed in doing this last night. Sorry about that, will hopefully get to this tonight!

@ljharb
Copy link
Contributor

ljharb commented Apr 3, 2015

@mikesherov np, any chance for today?

@ljharb
Copy link
Contributor

ljharb commented Apr 4, 2015

@mikesherov :-) ping again? (please stop me if this form of a reminder isn't helpful, not trying to be a bother)

@qfox
Copy link
Member

qfox commented Apr 4, 2015

@mikesherov Can I help you with merging cherry-picking this? ;-)

@mrjoelkemp
Copy link
Member

The pinging isn't necessary :) We don't usually forget about PRs, we just do a bunch of merges in one sitting.

@zxqfox, since it's been approved by two maintainers, it should be fine to initiate the fast-forward merge.

We'd still have to cut a patch release, which won't happen today. However, @ljharb, after merge, you can reference the master branch of jscs if you need immediate access to the fix.

@qfox qfox closed this in 93b7480 Apr 16, 2015
@qfox
Copy link
Member

qfox commented Apr 16, 2015

@ljharb Landed, thanks for patience 😼

@ljharb
Copy link
Contributor

ljharb commented Apr 17, 2015

Thank you!!! I'll wait quietly for the publish :-)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

v1.12.0 bug: disallowIdentifierNames rule disallows toString, valueOf, propertyIsEnumerable, __proto__
7 participants