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

i-bem: setMod/hasMod convert non-boolean values to string #893

Merged
merged 1 commit into from
Mar 7, 2015
Merged

Conversation

narqo
Copy link
Member

@narqo narqo commented Mar 3, 2015

closes #890

/cc @dfilatov

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 83.52% when pulling 47414ea on issues/890@v2 into 7947ae6 on v2.

@dfilatov
Copy link
Member

dfilatov commented Mar 3, 2015

commit title doesn't correspond its content

@narqo narqo changed the title i-bem: setMod should cast non-boolean value to string i-bem: setMod should convert non-boolean value to string Mar 3, 2015
@dfilatov
Copy link
Member

dfilatov commented Mar 3, 2015

I have two points about that:

  • jsdoc said that it took String param. So passing Number to it can't be treated as a bug.
  • we have only special case —Boolean, so we should explicitly cast to String any other values, not just Number.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 83.52% when pulling 25ff8d1 on issues/890@v2 into 7947ae6 on v2.

@dfilatov
Copy link
Member

dfilatov commented Mar 4, 2015

@narqo ping

@narqo
Copy link
Member Author

narqo commented Mar 4, 2015

jsdoc said that it took String param. So passing Number to it can't be treated as a bug.

Agree with it. But at the same time I'd prefer to have a warnings (or an error) about such things. Currently it not that obvious, how that code works.

[..] so we should explicitly cast to String any other values, not just Number.

I think I haven't understood your point. Which other types in addition to String and Number do you mean?

BTW, we've fixed jsdoc of these methods as well.

@dfilatov
Copy link
Member

dfilatov commented Mar 4, 2015

I think I haven't understood your point. Which other types in addition to String and Number do you mean?

I mean we don't need to explicitly consider Number as a special case. Boolean (and maybe undefined) is only special case. For any other values we should just call .toString().

@qfox
Copy link
Member

qfox commented Mar 4, 2015

Propose {Boolean|*} for type (or {?} if we want to do here #893). And description: true or undefined would be treated as "true" string.
But what about false?

@dfilatov
Copy link
Member

dfilatov commented Mar 5, 2015

No, undefined should not be treated as true, especially as string, setMod(modName, undefined) is equivalent of deleting this mod.
In case of setMod(modName) modVal should be treated as Boolean true.

@qfox
Copy link
Member

qfox commented Mar 5, 2015

@dfilatov Sorry, I meant #892

@dfilatov
Copy link
Member

dfilatov commented Mar 5, 2015

@zxqfox Do you ever sleep? )

@qfox
Copy link
Member

qfox commented Mar 5, 2015

@dfilatov Should I? ;-)

@narqo narqo changed the title i-bem: setMod should convert non-boolean value to string i-bem: setMod/hasMod convert non-boolean values to string Mar 5, 2015
@narqo
Copy link
Member Author

narqo commented Mar 5, 2015

🆙

@narqo narqo force-pushed the issues/890@v2 branch 2 times, most recently from 9c77691 to 7790b95 Compare March 7, 2015 11:40
@narqo
Copy link
Member Author

narqo commented Mar 7, 2015

@dfilatov pong

typeModVal === 'boolean' ||
typeModVal === 'undefined' || (modVal = modVal.toString());

var res = this.getMod(elem, modName) === modVal;
Copy link
Member

Choose a reason for hiding this comment

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

double space before modVal

@dfilatov
Copy link
Member

dfilatov commented Mar 7, 2015

lgtm

@dfilatov
Copy link
Member

dfilatov commented Mar 7, 2015

don't forget about v3, will you?

narqo pushed a commit that referenced this pull request Mar 7, 2015
i-bem: setMod/hasMod convert non-boolean values to string
@narqo narqo merged commit 2ee4ce6 into v2 Mar 7, 2015
@narqo narqo deleted the issues/890@v2 branch March 7, 2015 20:14
narqo pushed a commit that referenced this pull request May 6, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants