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

length as assertion => lengthOf #841

Closed
wants to merge 2 commits into from
Closed

length as assertion => lengthOf #841

wants to merge 2 commits into from

Conversation

shvaikalesh
Copy link
Contributor

Related to #684.

@meeber
Copy link
Contributor

meeber commented Oct 11, 2016

I have some reservations about deprecating length as an assertion but not deprecating length as a language chain / flag, given that they're both affected by the same problem. I'd like some time to think about it and organize my thoughts.

@keithamus
Copy link
Member

Well, the code in this PR LGTM. Let's get this one merged (as it's going towards fixing the issue) and move discussion about whether or not we should deprecate length wholesale back into #684

@lucasfcosta
Copy link
Member

lucasfcosta commented Oct 11, 2016

This LGTM too, great work @shvaikalesh.
I'll not merge this yet so @meeber can have a chance to do it if he has no urgent concerns about it.

@meeber
Copy link
Contributor

meeber commented Oct 17, 2016

Let's table this for post-4.0?

@lucasfcosta
Copy link
Member

@meeber Agreed. First things first 😄

@keithamus
Copy link
Member

Shall we squeeze this in before 4.0 final? Canary is released so we can chuck this one in right?

@meeber
Copy link
Contributor

meeber commented Nov 25, 2016

As discussed in #684, I'm opposed to deprecating .length; I believe the overall cost of doing so is way higher than the benefit. But if the other contributors feel strongly that it should be deprecated anyway, then this PR needs to be updated to also deprecate .length as a language chain; the issue being addressed affects .length both as an assertion and as a language chain. Note that deprecating .length as a language chain will leave some awkward phrasing behind, since we have examples out there like expect(blah).to.have.length.below(5) which don't translate very well to expect(blah).to.have.lengthOf.below(5).

@vieiralucas
Copy link
Member

I'm with @meeber here.

I agree that the cost is way higher than the benefit.

One consequence of removing length completely is that many Chai examples on the net will no longer work. The confusion experienced by new developers trying to use length based off such examples (or off of intuition) may be greater than they would experience in the event that they tried using have.a.length and didn't understand the error message.

/\ This is very important to me, as @meeber said:

if Chai were a young project, there would be virtually no cost in doing so, making it the clearly correct choice.

IMHO if we deprecate length as assertion, we should deprecate it as a language chain too. And since lengthOf doesn't work everywhere, what do you think of using a synonym like size, or maybe short it to len?

@lucasfcosta
Copy link
Member

Hey everyone, sorry for the delay, I spent the whole weekend traveling.
I agree with @vieiralucas, maybe we could use a synonym and size looks good to me.
As @meeber said, the cost would be way higher than the benefit, so we could just add another synonym and encourage our users to use it instead of using length.

We could also add a deprecation warning on 4.0.0 recommending our users to use size, for example, and then remove length completely on 5.0.0.

@keithamus
Copy link
Member

I'd honestly like to keep length, but it's a footgun. We can make it work in ES6 environments, but not on older environs. Cross browser issues are particularly sensitive for us - as an assertion framework - as it can start to look like the user's cross browser errors.

Sadly even IE11 doesn't support setting length, so we're stuck with this problem for a long time.

@meeber
Copy link
Contributor

meeber commented Nov 28, 2016

I think we should just leave it as consistently broken across all environments until all environments we support are ES6-compliant, and then we can fix it. It's such a low-impact bug because it only causes a failure in rare circumstances, and it fails loudly instead of silently. But deprecating length entirely at this stage in the project is a high-impact solution; everyone will have to rewrite all of their tests using length, almost all of which were perfectly valid, and very little is gained as a result. After all, people will still attempt to use length even if it's deprecated, just based on intuition or existing code examples on the internet. The situation isn't ideal but the proposed solution is worse than the problem.

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.

5 participants