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

FindChildBlock throws an Error if Block given as string #1469

Merged
merged 1 commit into from
Mar 6, 2017

Conversation

skad0
Copy link
Contributor

@skad0 skad0 commented Jan 10, 2017

Referred to this

@tadatuta

@coveralls
Copy link

Coverage Status

Coverage increased (+0.08%) to 62.313% when pulling 6278dd8 on skad0:issues/1464@v4 into a4febb6 on bem:v4.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.08%) to 62.313% when pulling 6278dd8 on skad0:issues/1464@v4 into a4febb6 on bem:v4.

@tadatuta
Copy link
Member

@skad0 should also throw on { block: 'string' }

@@ -442,7 +442,10 @@ var BemDomEntity = inherit(/** @lends BemDomEntity.prototype */{
* @returns {Block}
*/
findChildBlock : function(Block) {
// TODO: throw if Block passed as a string
if(typeof Block === 'string') {
throw new Error('Block can\'t be a String');
Copy link
Member

@narqo narqo Jan 25, 2017

Choose a reason for hiding this comment

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

The message which describes what type Block is allowed to be, might be more useful in case of error. Consider "Block must be a class or an instance", as an example.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.08%) to 62.313% when pulling 8df32d2 on skad0:issues/1464@v4 into a4febb6 on bem:v4.

@skad0
Copy link
Contributor Author

skad0 commented Jan 26, 2017

🆙

@tadatuta
Copy link
Member

tadatuta commented Feb 9, 2017

cc @dfilatov

@dfilatov
Copy link
Member

  • What about this check in others find*Block methods?
  • I think this check should be extracted to a separate function, something like validateBlockDesc and used in each find*Block method.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 62.391% when pulling bf77793 on skad0:issues/1464@v4 into 12659bd on bem:v4.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 62.391% when pulling 38f10e4 on skad0:issues/1464@v4 into 12659bd on bem:v4.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 62.41% when pulling 57f78c0 on skad0:issues/1464@v4 into 12659bd on bem:v4.

@skad0
Copy link
Contributor Author

skad0 commented Feb 21, 2017

🆙

@dfilatov
Copy link
Member

lgtm

typeof Block === 'object' && typeof Block.block === 'string'
) {
throw new Error('Block must be a class or description (block, modName, modVal) of the block to find');
}
Copy link

Choose a reason for hiding this comment

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

  • i still can pass primitives numbers, boolean, undefined
  • any function

error message of this validation and validation description don't fit with code.

@@ -442,7 +456,8 @@ var BemDomEntity = inherit(/** @lends BemDomEntity.prototype */{
* @returns {Block}
*/
findChildBlock : function(Block) {
// TODO: throw if Block passed as a string
validateBlockParam(Block);

return this._findEntities('find', Block, true);
Copy link

Choose a reason for hiding this comment

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

Why we don't do this check at _findEntities ?

Copy link
Member

Choose a reason for hiding this comment

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

'cos _findEntities is also used for elements passed as strings

@tadatuta tadatuta merged commit 3854cea into bem:v4 Mar 6, 2017
@skad0 skad0 deleted the issues/1464@v4 branch May 23, 2017 15:40
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.

None yet

6 participants