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

refactor: Use ::class instead of FQN-strings #212

Merged
merged 2 commits into from
Feb 15, 2024

Conversation

Niellles
Copy link
Contributor

No description provided.

Change namespaces for test classes to comply with PSR-4. As per PSR-4 2.3.2 the namespace should follow the projects directory structure (https://www.php-fig.org/psr/psr-4/).
As of PHP 5.5 SomeClass::class represents SomeClass' FQN. As support for <7.3 (thus <5.5) was dropped in meyfa#182, this notation can now be used.
@Niellles
Copy link
Contributor Author

I don't particularly care for the long list of use statements in src/Reading/NodeRegistry.php, however, my IDE collapses it by default, so I suppose it's alright (for me).

@meyfa
Copy link
Owner

meyfa commented Dec 13, 2023

Very nice, this is great!

Would it be possible & acceptable to avoid all the imports in src/Reading/NodeRegistry.php by fully qualifying the class names, e.g.:

'line' => \SVG\Nodes\Shapes\SVGLine::class

This should lead to some amount of type safety since it's possible to statically verify the class existence, instead of it being a string. I'm not sure, however, if this is possible in PHP and whether it would lead to IDEs complaining about the FQDN and suggesting to add the imports.

@meyfa
Copy link
Owner

meyfa commented Feb 6, 2024

Hey @Niellles, I hope you're doing well. Did you have time to look into my previous comment? If not that's alright as well, I might merge this PR as-is, then. WDYT?

@tacman
Copy link
Contributor

tacman commented Feb 6, 2024

'line' => \SVG\Nodes\Shapes\SVGLine::class

I prefer having the use statements, in fact I think some of the cleanup scripts (rector or php-cs, I forget which) will refactor them.

@Niellles
Copy link
Contributor Author

Niellles commented Feb 8, 2024

@meyfa Doing great, thanks for asking :). I've been somewhat busy.

Personally I prefer the use statements. However, I was thinking maybe the node types should be enums. Not that we can use PHPs native "real" enums (although we could if we fix #215).. can you take a look at: Niellles#2?

@meyfa meyfa changed the title refactor: Fix tests namespaces + Use ::class instead of FQN-strings refactor: Use ::class instead of FQN-strings Feb 15, 2024
@meyfa meyfa merged commit da6dca5 into meyfa:main Feb 15, 2024
8 checks passed
@meyfa
Copy link
Owner

meyfa commented Feb 15, 2024

@Niellles I don't know about the enum refactor. What's the benefit over the current design?

@meyfa
Copy link
Owner

meyfa commented Feb 15, 2024

Thanks for your work on this by the way, and sorry it has taken so long to get merged.

tacman added a commit to tacman/php-svg that referenced this pull request Feb 15, 2024
@meyfa
Copy link
Owner

meyfa commented Mar 17, 2024

Just published v0.15.0 including these changes 🎉

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

3 participants