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

Add test case for empty path #31

Closed
wants to merge 1 commit into from
Closed

Conversation

djfhe
Copy link
Member

@djfhe djfhe commented May 20, 2024

Add some test cases to ensure correct handling of an empty path.

@djfhe djfhe force-pushed the add-test-case-for-empty-path branch from 4d7e45d to 576572e Compare May 20, 2024 17:58
@djfhe djfhe requested a review from saibotk May 20, 2024 18:12
foo: 'bar',
} as const

// @ts-expect-error - '' is not define on the property, so it is correct that typescript throws an error here
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// @ts-expect-error - '' is not define on the property, so it is correct that typescript throws an error here
// @ts-expect-error - '' is not defined on the object, so it is correct that typescript throws an error here

// but as mentioned above, we still will set the value at runtime
setByPath(test, '', 'bar')

// @ts-expect-error - '' is not set on the property, so typescript should throw an error here
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// @ts-expect-error - '' is not set on the property, so typescript should throw an error here
// @ts-expect-error - '' is not set on the object, so typescript should throw an error here

Comment on lines +122 to +124
// @ts-expect-error - 'foo' is not set on the property, so typescript should throw an error here
// but as mentioned above, we still will set the value at runtime
setByPath(test, '', 'bar')
Copy link
Member

Choose a reason for hiding this comment

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

this and the one line afterwards should rather get their own test case. The test name only implies that the access to an unset path is tested

@@ -110,6 +110,24 @@ it('returns undefined on non objects', () => {
expect(getByPath(test, 'undef.baz')).toBe(undefined)
})

it('it returns undefined when accessing an unset path', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
it('it returns undefined when accessing an unset path', () => {
it('returns undefined when accessing an unset path', () => {

@saibotk saibotk added the skip-changelog Skips changelog check label May 26, 2024
@djfhe
Copy link
Member Author

djfhe commented Jun 2, 2024

i would close this PR in favor of #11. I don't see any use in merging this PR right before 2.0 where we change this behavior anyways. @saibotk

@djfhe djfhe closed this Jun 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip-changelog Skips changelog check
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants