Skip to content

replace assertEmpty() with stricter assertions #59898

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
Merged
merged 1 commit into from
Mar 5, 2025
Merged

replace assertEmpty() with stricter assertions #59898

merged 1 commit into from
Mar 5, 2025

Conversation

xabbuh
Copy link
Member

@xabbuh xabbuh commented Mar 3, 2025

Q A
Branch? 7.3
Bug fix? no
New feature? no
Deprecations? no
Issues
License MIT

In the code we are strict about not using empty(). In my opinion it makes sense to be equally strict with our assertions. But I'd like to gather some feedback first before updating all places.

@carsonbot carsonbot added Status: Needs Review RFC RFC = Request For Comments (proposals about features that you want to be discussed) labels Mar 3, 2025
@carsonbot carsonbot added this to the 7.3 milestone Mar 3, 2025
@carsonbot carsonbot changed the title [RFC] replace assertEmpty() with stricter assertions replace assertEmpty() with stricter assertions Mar 3, 2025
@xabbuh xabbuh added Status: Needs Work and removed RFC RFC = Request For Comments (proposals about features that you want to be discussed) Status: Needs Review labels Mar 3, 2025
@fabpot
Copy link
Member

fabpot commented Mar 3, 2025

👍 As we avoid using empty() in the code base, it makes a lot of sense to me.

@OskarStark OskarStark changed the title replace assertEmpty() with stricter assertions replace assertEmpty() with stricter assertions Mar 3, 2025
@xabbuh xabbuh force-pushed the drop-assert-empty branch from 105cb0b to 3c26d6e Compare March 5, 2025 08:08
@xabbuh xabbuh requested a review from dunglas as a code owner March 5, 2025 08:08
@xabbuh
Copy link
Member Author

xabbuh commented Mar 5, 2025

please do not merge yet, I haven't updated all places

@xabbuh xabbuh force-pushed the drop-assert-empty branch from 3c26d6e to bb030a7 Compare March 5, 2025 08:42
@xabbuh xabbuh force-pushed the drop-assert-empty branch from bb030a7 to 6550cb8 Compare March 5, 2025 10:15
@xabbuh
Copy link
Member Author

xabbuh commented Mar 5, 2025

ready

Status: Needs review

OskarStark
OskarStark approved these changes Mar 5, 2025
View reviewed changes
Copy link
Contributor

@OskarStark OskarStark left a comment

Choose a reason for hiding this comment

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

@fabpot shall we teach fabbot to report such things in future?

@fabpot
Copy link
Member

fabpot commented Mar 5, 2025 •
edited
Loading

@fabpot shall we teach fabbot to report such things in future?

I suppose this could be part of PHP-CS-Fixer?

@fabpot
Copy link
Member

fabpot commented Mar 5, 2025

Thank you @xabbuh.

@fabpot fabpot merged commit a9a1226 into symfony:7.3 Mar 5, 2025
13 checks passed
@xabbuh xabbuh deleted the drop-assert-empty branch March 5, 2025 11:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabpot fabpot fabpot approved these changes

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@OskarStark OskarStark OskarStark approved these changes

@mtarld mtarld mtarld approved these changes

@chalasr chalasr chalasr approved these changes

@dunglas dunglas Awaiting requested review from dunglas dunglas is a code owner

@lyrixx lyrixx Awaiting requested review from lyrixx lyrixx is a code owner

@welcoMattic welcoMattic Awaiting requested review from welcoMattic welcoMattic is a code owner

@kbond kbond Awaiting requested review from kbond kbond is a code owner

@yceruto yceruto Awaiting requested review from yceruto yceruto is a code owner

@jderusse jderusse Awaiting requested review from jderusse jderusse is a code owner

Assignees
No one assigned
Labels
Status: Reviewed
Projects
None yet
Milestone
7.3
Development

Successfully merging this pull request may close these issues.
7 participants
@xabbuh @fabpot @nicolas-grekas @OskarStark @mtarld @chalasr @carsonbot