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

Unify property extractor function names #524

Merged
merged 11 commits into from
Apr 18, 2024

Conversation

grodin
Copy link
Contributor

@grodin grodin commented Mar 20, 2024

Closes #521

I'm opening this as a draft as the docs aren't updated yet and this is just supposed to be to get the bikeshedding going.

Known things to fix before merge:

  • update docs
  • update README
  • update CHANGELOG
  • update names of tests
  • deprecate old functions

@evant
Copy link
Collaborator

evant commented Mar 25, 2024

While we are still bike-shedding the naming I want to call out we'll want to keep but deprecate the old names, not get rid of them entirely.

@grodin
Copy link
Contributor Author

grodin commented Mar 26, 2024

Yeah, I realised that after I pushed those commits. I was a bit too enthusiastic to get the bike-shedding going I think.

I'll add that to the to-do list above.

@grodin grodin force-pushed the grodin/unify-prop-extractors branch from 761c293 to 8c74fce Compare April 18, 2024 12:57
@grodin grodin mentioned this pull request Apr 18, 2024
@grodin grodin force-pushed the grodin/unify-prop-extractors branch from 3c12edf to 770d37d Compare April 18, 2024 14:31
@grodin grodin marked this pull request as ready for review April 18, 2024 14:53
@grodin grodin requested review from evant, rf43 and nishtahir as code owners April 18, 2024 14:53
@grodin
Copy link
Contributor Author

grodin commented Apr 18, 2024

Let me know if you want this squashed

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@evant evant left a comment

Choose a reason for hiding this comment

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

other than that looks good! While this should be squashed up to you if you want to do that, I can also do it while merging.

README.md Outdated Show resolved Hide resolved
@grodin grodin force-pushed the grodin/unify-prop-extractors branch from 3af4e67 to 0291fa5 Compare April 18, 2024 22:11
@grodin
Copy link
Contributor Author

grodin commented Apr 18, 2024

I'll let you squash it

@evant evant enabled auto-merge (squash) April 18, 2024 22:14
@evant evant merged commit 6b65668 into willowtreeapps:main Apr 18, 2024
4 of 5 checks passed
@grodin grodin deleted the grodin/unify-prop-extractors branch April 18, 2024 22:16
* ```
*/
suspend fun <T, P> Assert<T>.having(name: String, extract: suspend (T) -> P): Assert<P> =
Copy link
Contributor

@JakeWharton JakeWharton Apr 18, 2024

Choose a reason for hiding this comment

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

If we make the other one inline, we can eliminate this psuedo-overload. Do we want that? Both APIs used in the implementation are public.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah doing that would be fine

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.

Consider renaming/unifying prop/extracting/suspendCall
3 participants