-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Issue #6216 -- have .its() receive the number 0 as parameter #6234
Conversation
Thanks for the contribution! Below are some guidelines Cypress uses when doing PR reviews.
PR Review ChecklistIf any of the following requirements can't be met, leave a comment in the review selecting 'Request changes', otherwise 'Approve'. User Experience
Functionality
Maintainability
Quality
Internal
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for opening this PR! The changes look good, but I noticed a few minor things that should be fixed before this is merged.
packages/driver/test/cypress/integration/commands/connectors_spec.coffee
Show resolved
Hide resolved
The check for index - what does it do if one passes “-1”? Ughh JavaScript allows so much stuff
…Sent from my iPhone
On Jan 24, 2020, at 10:22, Zach Bloomquist ***@***.***> wrote:
@flotwig requested changes on this pull request.
Thanks for opening this PR! The changes look good, but I noticed a few minor things that should be fixed before this is merged.
In packages/driver/test/cypress/integration/commands/connectors_spec.coffee:
> @@ -813,6 +813,11 @@ describe "src/cy/commands/connectors", ->
it "works with numerical indexes", ->
cy.wrap(['foo', 'bar']).its(1).should('eq', 'bar')
+ it "works with 0 as a value if object has property 0", ->
add a test for cy.invoke, since it uses the same connectors logic, cy.invoke(0) should work too
In packages/driver/src/cy/commands/connectors.coffee:
> @@ -179,7 +179,8 @@ module.exports = (Commands, Cypress, cy, state, config) ->
consoleProps: ->
Subject: subject
- if not str
+ ## check for false positive (negative?) with 0 given as index
+ if not str and str isnt 0
since this logic is shared between line 256 and this line, move it to a named function for clarity. maybe something like this:
isObjectKey = (i) =>
i or i is 0
In packages/driver/src/cy/commands/connectors.coffee:
> @@ -251,9 +252,12 @@ module.exports = (Commands, Cypress, cy, state, config) ->
previousProp = pathsArray[index - 1]
valIsNullOrUndefined = _.isNil(acc)
+ ## to allow the falsy value 0 to be used
+ propIsValid = !!prop or prop is 0
see comment on line 183
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
@bahmutov |
It looks like you made the first commits with a gmail email address, then you made the last two commits with an email address (contact [at] datner [dot] me) which is not yet linked to your GitHub account. Add this email to your GitHub account and confirm it, then ask the CLA assistant to retry, it should work once the 2nd email is associated with your GitHub: https://github.com/settings/emails
Yup, negative indices are valid, this is good. |
Thanks! |
Very odd.. I never used that email for git. But done |
User facing changelog
its()
now supports 0 as a value for indexes or object keysAdditional details
Many times a developer will intuitively reach for the first item in an array for tests or checks
(or for anything if I'm honest), and until now trying to reach that property using
.its(0)
threw an error.Now developers world-wide would be able to check the first index of their array in a cypress environment. Well, I'm sure you could still have done it with
.its("0")
but just isn't as cool.The tests check for edge cases, other than that I didn't do anything unexpected
How has the user experience changed?
this didn't work, now it does, it's the intuitive thing to do, so it's pretty chill
PR Tasks
cypress-documentation
?Is there anything to add to cypress-documentation? This now works as planned, I didn't add any new features to the mental model of how
.its()
operates