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

chore: Use a dedicated SVG\Tests namespace #225

Merged
merged 2 commits into from
Mar 28, 2024

Conversation

smnandre
Copy link
Contributor

This one is really a "let's how that'd look" ... i could 100% understand you're used to the current system

I do think it's better to avoid mixing namespaces / folders, but as it's only in dev.. maybe not that important

Well if you wanted to... it's ready :)

This one is really a "let's how that'd look" ... i could 100% understand you're used to the current system

I do think it's better to avoid mixing namespaces / folders, but as it's only in dev.. maybe not that important

Well if you wanted to... it's ready :)
@meyfa
Copy link
Owner

meyfa commented Mar 17, 2024

I don't have strong opinions about this, but it seems like the current system is the more "traditional" way that even PHPUnit itself follows (see e.g.: https://github.com/sebastianbergmann/phpunit/blob/main/tests/unit/Framework/Constraint/CallbackTest.php).

It looks nicer in the composer.json the way this PR proposes, but in the end the result is the same, except the class under test must be imported explicitly.

For now I would somewhat favor avoiding the code churn by keeping the namespace as it is.

@smnandre
Copy link
Contributor Author

@smnandre
Copy link
Contributor Author

PHPunit is not a good exemple for me, as it itself is a test Library :)

But if you look other popular PHP library, the test namespace is really something common.

It is up to you, but as it’s kinda « now or never » (you wont do it after 1.0 i guess) i tried :)

@smnandre
Copy link
Contributor Author

(Symfony, Doctrine, Laravel)

@meyfa
Copy link
Owner

meyfa commented Mar 17, 2024

Thanks for the explanation.

you wont do it after 1.0 i guess

As this only concerns the development but not any production use of PHP-SVG, I think we could do it at any point, since nobody using PHP-SVG as a dependency would be affected.

Regarding the arguments in that article:

You document the namespace for tests for yourself and other developers.

This is true no matter which style we use.

You document the namespace and allow autoloading of abstract test cases, test helpers, test doubles, data providers, and other helpful objects.

Same here.

You document the namespace for your favorite IDE.

I don't know about that since I don't use PHPStorm like the author, but it seems the same rules apply here as well.


Overall, the article only seems in favor of adding an autoload-dev section (which we already have), not about a particular namespace mapping.


I see your point about other libraries. Maybe we can merge this PR on the basis that it helps new contributors due to a familiar directory structure.

@smnandre
Copy link
Contributor Author

I had little hope for this one to be merged to be honest, so i'm really following your decision there :))

I added links and example that i should have written in the PR comment in the first place.

So i'm really not pushing to do something you'd not be comfortable with :)

@meyfa meyfa changed the title [DX] Use a dedicated SVG\Tests namespace chore: Use a dedicated SVG\Tests namespace Mar 28, 2024
@meyfa
Copy link
Owner

meyfa commented Mar 28, 2024

After thinking about this some more, aligning ourselves with popular projects seems like the right thing to do.

I'll go ahead and merge this, thanks for the contribution!

@meyfa meyfa merged commit e62e8ed into meyfa:main Mar 28, 2024
7 checks passed
@smnandre
Copy link
Contributor Author

After thinking about this some more, aligning ourselves with popular projects seems like the right thing to do.

I'll go ahead and merge this, thanks for the contribution!

It's a pleasure! Thank you for this project :)

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.

2 participants