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.

Sign up for GitHub

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

Jump to bottom

rename(std/testing): rename assert*Contains to assert*Includes #7951

Merged
merged 34 commits into from
Oct 26, 2020
Merged

rename(std/testing): rename assert*Contains to assert*Includes #7951

merged 34 commits into from
Oct 26, 2020

Conversation

timreichen
Copy link
Contributor

@timreichen timreichen commented Oct 13, 2020 •
edited
Loading

#6888

Rename functions in asserts.ts

  • assertStringContains -> assertStringIncludes
  • assertArrayContains -> assertArrayIncludes

@timreichen timreichen changed the title rename(std/testing): rename contains to includes rename(std/testing/asserts.ts): rename contains to includes Oct 14, 2020
@bartlomieju
Copy link
Member

I'm generally in favor of this change, the only point of discussion is original suggestion from #6888 if we should have both assertStringIncludes and assertArrayIncludes or just assertIncludes as implemented in this PR. Given that assertEquals is magical and works on all values makes assertIncludes favorable.

CC @kitsonk @nayeemrmn @caspervonb

@caspervonb
Copy link
Contributor

caspervonb commented Oct 21, 2020 •
edited
Loading

Difficult to see what has actually changed here with 232 changed files in the diff but in favour of generalising on a.includes(b) as long as we provide rich pretty printed diffs for both strings and array-likes (otherwise, if we don't provide a better diff assert(a.includes(b)) is just as good)

caspervonb
caspervonb reviewed Oct 21, 2020
View reviewed changes
std/testing/asserts.ts Outdated Show resolved Hide resolved
@nayeemrmn
Copy link
Collaborator

if we should have both assertStringIncludes and assertArrayIncludes or just assertIncludes as implemented in this PR

I think they should definitely be different functions. They have different and unrelated behaviour, which would be especially confusing since strings are iterable. The JS methods are namespaced to Array.prototype.includes() and String.prototype.includes(). Making them overloads is illogical and makes both the implementation and call sites harder to read.

@timreichen timreichen closed this Oct 22, 2020
@timreichen
Copy link
Contributor Author

@caspervonb The special casing is because of the overload. To avoid that, @nayeemrmn suggestion might be the better solution. Just a rename of contains to includes.

@timreichen timreichen reopened this Oct 22, 2020
@bartlomieju
Copy link
Member

if we should have both assertStringIncludes and assertArrayIncludes or just assertIncludes as implemented in this PR

I think they should definitely be different functions. They have different and unrelated behaviour, which would be especially confusing since strings are iterable. The JS methods are namespaced to Array.prototype.includes() and String.prototype.includes(). Making them overloads is illogical and makes both the implementation and call sites harder to read.

Thanks for being voice of reason @nayeemrmn. In this light I agree that we should just do the rename without merging those assertion. @timreichen could you please update the PR?

@timreichen
Copy link
Contributor Author

@bartlomieju already did:)

bartlomieju
bartlomieju requested changes Oct 22, 2020
View reviewed changes
Releases.md Outdated Show resolved Hide resolved
@bartlomieju
Copy link
Member

@timreichen this PR shouldn't change 3k lines diff - I think something went wrong during rebase, please check

@timreichen
Copy link
Contributor Author

@bartlomieju Shoot, I suck at rebasing. Sorry. Done now.

bartlomieju
bartlomieju reviewed Oct 25, 2020
View reviewed changes
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM and I'm in favor of landing this PR

@lucacasonato @kitsonk please take a look

@lucacasonato
Copy link
Member

LGTM. We should definitely mention this in the blog post.

@bartlomieju bartlomieju changed the title rename(std/testing/asserts.ts): rename contains to includes rename(std/testing): rename assert*Contains to assert*Includes Oct 26, 2020
@bartlomieju bartlomieju merged commit ae86cbb into denoland:master Oct 26, 2020
@bartlomieju bartlomieju mentioned this pull request Oct 28, 2020
Closed
@timreichen timreichen deleted the std-testing-asserts branch October 28, 2020 21:41
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 21, 2021
@timreichen
rename(std/testing): rename assert*Contains to assert*Includes (denol…
9fa2b93 
…and/deno#7951)

This commit renames two assertion functions to better align with JS API:
- assertStringContains -> assertStringIncludes
- assertArrayContains -> assertArrayIncludes
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 24, 2021
@timreichen
rename(testing): rename assert*Contains to assert*Includes (denoland/…
5d440ae 
…deno#7951)

This commit renames two assertion functions to better align with JS API:
- assertStringContains -> assertStringIncludes
- assertArrayContains -> assertArrayIncludes
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 24, 2021
@timreichen @caspervonb
rename(testing): rename assert*Contains to assert*Includes (denoland/…
52a0356 
…deno#7951)

This commit renames two assertion functions to better align with JS API:
- assertStringContains -> assertStringIncludes
- assertArrayContains -> assertArrayIncludes
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 24, 2021
@timreichen @caspervonb
rename(testing): rename assert*Contains to assert*Includes (denoland/…
0fc83e4 
…deno#7951)

This commit renames two assertion functions to better align with JS API:
- assertStringContains -> assertStringIncludes
- assertArrayContains -> assertArrayIncludes
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
@timreichen @caspervonb
rename(testing): rename assert*Contains to assert*Includes (denoland/…
21adbcf 
…deno#7951)

This commit renames two assertion functions to better align with JS API:
- assertStringContains -> assertStringIncludes
- assertArrayContains -> assertArrayIncludes
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
@timreichen @caspervonb
rename(testing): rename assert*Contains to assert*Includes (denoland/…
e5321e5 
…deno#7951)

This commit renames two assertion functions to better align with JS API:
- assertStringContains -> assertStringIncludes
- assertArrayContains -> assertArrayIncludes
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
@timreichen @caspervonb
rename(testing): rename assert*Contains to assert*Includes (denoland/…
1b920c2 
…deno#7951)

This commit renames two assertion functions to better align with JS API:
- assertStringContains -> assertStringIncludes
- assertArrayContains -> assertArrayIncludes
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
@timreichen @denobot
rename(testing): rename assert*Contains to assert*Includes (denoland/…
b3bd687 
…deno#7951)

This commit renames two assertion functions to better align with JS API:
- assertStringContains -> assertStringIncludes
- assertArrayContains -> assertArrayIncludes
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Feb 1, 2021
@timreichen @denobot
rename(testing): rename assert*Contains to assert*Includes (denoland/…
55a7adb 
…deno#7951)

This commit renames two assertion functions to better align with JS API:
- assertStringContains -> assertStringIncludes
- assertArrayContains -> assertArrayIncludes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@caspervonb caspervonb caspervonb left review comments

@bartlomieju bartlomieju bartlomieju approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@timreichen @bartlomieju @caspervonb @nayeemrmn @lucacasonato