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

console.assert should return a boolean #63

Closed
shelby3 opened this issue Aug 4, 2016 · 10 comments
Closed

console.assert should return a boolean #63

shelby3 opened this issue Aug 4, 2016 · 10 comments
Labels
addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest

Comments

@shelby3
Copy link

shelby3 commented Aug 4, 2016

The don't repeat yourself (DRY) principle of software development says that:

console.assert(_ instanceof Promise)
if (!(_ instanceof Promise)) reject()

Is remiss compared to:

if (console.assert(_ instanceof Promise)) reject()

@domenic domenic added the needs implementer interest Moving the issue forward requires implementers to express interest label Aug 4, 2016
@shelby3
Copy link
Author

shelby3 commented Aug 4, 2016

Untested, proposed ES6 polyfill:

console.assert = (function() {
    'use strict'
    const assert = console.assert
    return function(...args) {
      assert.apply(args, console)
      return !args[0]
    }
  })()

To remove the ES6 dependency:

console.assert = (function() {
    'use strict'
    const assert = console.assert
    return function() {
      assert.apply(arguments, console)
      return !arguments[0]
    }
  })()

@Fishrock123
Copy link

In most cases would it not be cleaner to just assign the result of the conditional to a variable / constant and log/check that?

@guimier
Copy link

guimier commented Jan 16, 2017

@shelby3 assert.apply(console, args)

And I would rather write if (!console.assert(…)): returning true in case of failure seems weird.

@domfarolino
Copy link
Member

domfarolino commented Jun 7, 2017

I tend to agree with @Fishrock123 on this. I think there are valid cases for console methods to return a value (tracked here #75) and my thoughts on this are in #75 (comment), specifically point # 2. I could have an unpopular opinion on this but I think that this wouldn't be terribly useful/helpful, and furthermore is kind of non-obvious.

It also feels weird to pepper my code/conditionals with console.assert(someBusinessLogic) where someBusinessLogic would normally appear alone, but maybe that's just me. Like personally I prefer

console.assert(something);
if (something) {
  doThis();
}
//...

and if (console.assert(...)) just feels odd, especially since console.assert() doesn't throw an error and break execution (and this probably won't change #77)

Thoughts?

@terinjokes
Copy link
Collaborator

My preference would be to avoid changing console.assert and move to something that has the behavior people expect (throwing).

That might be a Console method or something else.

@domfarolino
Copy link
Member

That makes sense to me. It would also bring the browser's version of assert further away from Node's implementation, giving developers the illusion that that they can get away with using the return value of assert in JavaScript (remember, not the case in Node).

So while my opinion leans toward closing this, I wouldn't mind hearing from some implementers /cc @paulirish @JosephPecoraro @xirzec for some final feedback.

@paulirish
Copy link

We'd also prefer to keep console.assert as is, and not change the return value semantics. The polyfill solution above seems reasonable for people that want the syntactic sugar.

@domfarolino
Copy link
Member

Agreed, I'm going to close this issue. Thanks @shelby3 for filing, and providing the polyfill!

@xirzec
Copy link

xirzec commented Jan 15, 2018

I'm late to the party, but I'd prefer keeping it as-is as well. Folks have articulated some good arguments, and I have some selfish (implementation specific) reasons for preferring it to not return anything.

@shelby3
Copy link
Author

shelby3 commented Jun 3, 2018

In most cases would it not be cleaner to just assign the result of the conditional to a variable / constant and log/check that?

Why should we decide what other programmers prefer in this case? What harm comes from giving them this choice?

And I would rather write if (!console.assert(…)): returning true in case of failure seems weird.

That alternative would be okay with me. My brain can grok it either way.

I could have an unpopular opinion on this but I think that this wouldn't be terribly useful/helpful, and furthermore is kind of non-obvious.

Just curious why any of us should feel we should decide what other programmers want to use. Those who do not want to use the return value would not be forced to use by its existence. But without its existence those who want to use it, can’t.

My preference would be to avoid changing console.assert and move to something that has the behavior people expect (throwing).

I sort of agree, but it’s console.assert not assert. I would have preferred if it was named console.logiffalse or even preferred console.logif with it logging if value is true. I personally prefer Hungarian notation console.logIf for clarity, but I’m not proposing that here since it seems to not be the cultural norm in the EMCAScript ecosystem.

That makes sense to me. It would also bring the browser's version of assert further away from Node's implementation, giving developers the illusion that that they can get away with using the return value of assert in JavaScript (remember, not the case in Node).

console.assert is not an API of EMCAScript is it? So why would they get confused other than if they just refuse to read documentation?

Why are we making any decisions based on what Node does. It’s just one of many possible platforms that integrate with EMCAScript. There are many browser APIs that are not available on Node.

We'd also prefer to keep console.assert as is, and not change the return value semantics. The polyfill solution above seems reasonable for people that want the syntactic sugar.

That’s true. And anyway who will be coding in JavaScript anymore. We’ll all be using some PL that compiles to JavaScript or WASM soon anyway.

Agreed, I'm going to close this issue. Thanks @shelby3 for filing, and providing the polyfill!

Yw. Cheers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest
Development

No branches or pull requests

8 participants