Skip to content

Conversation

@alissn
Copy link
Contributor

@alissn alissn commented Aug 9, 2025

Hi,

This pull request fixes #2096.
The getTableName method’s return type can be null, so its type declaration has been updated.

@alissn
Copy link
Contributor Author

alissn commented Aug 9, 2025

@solomon-ochepa

I’m sure that changing without adding tests can cause new issues.
Your latest pull request seems to follow this pattern.

Currently, this package has only 64% test coverage, so we can’t be sure all the changes you made are tested and will work fine.

@solomon-ochepa
Copy link
Contributor

@solomon-ochepa

I’m sure that changing without adding tests can cause new issues. Your latest pull request seems to follow this pattern.

Currently, this package has only 64% test coverage, so we can’t be sure all the changes you made are tested and will work fine.

Yes, I agree. However, as you’ve stated, the coverage is poor, not that tests were not written.

Could you please add tests for this new change?

@alissn
Copy link
Contributor Author

alissn commented Aug 10, 2025

@solomon-ochepa

Rather than only updating the stub file and renaming methods, it would be more valuable to contribute by adding additional test coverage or developing new features, as this would have a greater positive impact on the project.

This PR doesn’t require tests. Refactoring this part would be better, but I don’t have the time right now. If you can, please create a new PR for that.

@solomon-ochepa
Copy link
Contributor

@solomon-ochepa

Rather than only updating the stub file and renaming methods, it would be more valuable to contribute by adding additional test coverage or developing new features, as this would have a greater positive impact on the project.

Debugging is a crucial part of keeping a clean, readable, and maintainable codebase. If the code had been written properly from the start, there wouldn’t be a need to update it later. Remember, what’s worth doing is worth doing well!

The stubs, naming convention, and consistency are the foundation.

This PR doesn’t require tests. Refactoring this part would be better, but I don’t have the time right now. If you can, please create a new PR for that.

Sorry, but it does. It needs tests to verify that the method can be accessed, can return a string, and can return null.

@solomon-ochepa
Copy link
Contributor

This PR doesn’t require tests. Refactoring this part would be better, but I don’t have the time right now. If you can, please create a new PR for that.

Suggested Tests for getTableName(): ?string

  1. Returns correct string when matches exist
  • Setup a scenario where getMatches() returns a non-empty array (e.g., ['foo', 'bar', 'baz']).
  • Ensure getTableName() returns the first element after reversing—i.e., 'foo'.
  1. Returns null when the matches array is empty
  • Mock getMatches() to return an empty array ([]).
  • Confirm that getTableName() returns null.
  1. Correct behavior when matches only contain one element
  • getMatches() returns something like ['only'].
  • Expect getTableName() to return 'only'.
  1. Behavior with null values inside matches
  • If getMatches() returns arrays including nulls, e.g., ['one', null, 'two'].
  • Ensure getTableName() handles it gracefully (i.e., returns whatever array_shift gives—likely 'one', but test that explicitly).
  1. Access visibility & method chaining
  • Test that getTableName() is publicly accessible and works via the class interface without errors.

@alissn
Copy link
Contributor Author

alissn commented Aug 10, 2025

@solomon-ochepa

For more information, please check the linked issue and run the tests on your local environment. If you have suggestions for additional tests, feel free to create a new pull request.

I’m not interested in arguing, but I’m happy to answer questions from anyone except you.

@solomon-ochepa
Copy link
Contributor

Bro, no one is arguing with you here.

Quality over quantity, please - these are necessary tests.

We are here to collaborate, not to fight over every minor case.

Please, have a great day.

@dcblogdev dcblogdev merged commit 367aa66 into nWidart:master Sep 3, 2025
6 checks passed
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.

Nwidart\Modules\Support\Migrations\NameParser::getTableName(): Return value must be of type string, null returned

3 participants