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 hasRecordComponents() Class assertion #2995

Merged

Conversation

ljrmorgan
Copy link
Contributor

Check List:

Add a hasRecordComponents() Class assertion, following on from #2968.

I've tried to take a similar approach to assertions like hasPublicFields(), but as with isRecord() and isNotRecord() I've left the implementation of the assertion in AbstractClassAssert rather than extracting it to Classes.

Copy link
Member

@scordio scordio left a comment

Choose a reason for hiding this comment

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

Thanks @ljrmorgan, just two comments

@scordio scordio added this to the 3.25.0 milestone Mar 28, 2023
@ljrmorgan
Copy link
Contributor Author

Thanks for reviewing @scordio!

I was trying to copy the behaviour of hasPublicFields()/hasDeclaredFields() etc. which when they are called with no arguments assert that the class has no public/declared field. TBH though I found that behaviour a bit surprising, and I prefer your suggestion of just requiring that at least one parameter is passed to hasRecordComponents() (presumably by changing the signature to something like hasRecordComponents(String first, String... rest)?)

I'm less sure about adding a hasNoRecordComponents() assertion though. I can add it if you feel strongly, but I'm wondering if that's actually a useful thing to assert? I can understand why people might want to assert that a class has no public fields, I can't really think of a scenario where someone would to check that a record has no record components (or even why someone would want to create a record with no record components). I'm happy to go with whatever you think though.

Thanks!

@scordio
Copy link
Member

scordio commented Mar 30, 2023

presumably by changing the signature to something like hasRecordComponents(String first, String... rest)?

That could work, or also keeping a single vararg but making sure it's not null and has at least one element, checks that can be done in the method body.

I'm less sure about adding a hasNoRecordComponents() assertion though.

Fair enough, nothing prevents us to add it later if users come back with a concrete use case. For now, let's skip it.

Change hasRecordComponents() to require at least one record component to
be passed in
@ljrmorgan ljrmorgan force-pushed the add_has_record_components_assertions branch from aa62a4a to 2bbc02b Compare March 31, 2023 10:45
@ljrmorgan ljrmorgan requested a review from scordio April 3, 2023 12:11
# Conflicts:
#	assertj-core/src/main/java/org/assertj/core/api/AbstractClassAssert.java
@scordio scordio merged commit 44c18fe into assertj:main Apr 5, 2023
10 checks passed
@scordio
Copy link
Member

scordio commented Apr 5, 2023

This is now merged. Thanks, @ljrmorgan!

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.

None yet

2 participants