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

Create alternative to namespaceMatchesOneOfTheseNamespaces using splat instead of array #403

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

hgraca
Copy link
Contributor

@hgraca hgraca commented Jul 29, 2023

This provides better typing and ease of use.

This is, however, a BC break.

@codecov-commenter
Copy link

Codecov Report

Merging #403 (70eb7a2) into main (976c200) will not change coverage.
The diff coverage is 100.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff            @@
##               main     #403   +/-   ##
=========================================
  Coverage     94.33%   94.33%           
  Complexity      571      571           
=========================================
  Files            67       67           
  Lines          1500     1500           
=========================================
  Hits           1415     1415           
  Misses           85       85           
Files Changed Coverage Δ
src/Analyzer/ClassDescription.php 100.00% <100.00%> (ø)
src/Rules/ArchRule.php 100.00% <100.00%> (ø)

@@ -36,7 +36,7 @@ public function test_should_return_true_if_there_class_is_in_namespace_array():
{
$cd = $this->builder->build();

$this->assertTrue($cd->namespaceMatchesOneOfTheseNamespaces(['Fruit']));
$this->assertTrue($cd->namespaceMatchesOneOfTheseNamespaces('Fruit'));

Choose a reason for hiding this comment

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

I would suggest adding at least another string to show that we can add more arguments

Choose a reason for hiding this comment

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

@hgraca Can we continue to support array and the new behavior to not create a BC?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would suggest adding at least another string to show that we can add more arguments

I added another string to the test case.

Can we continue to support array and the new behavior to not create a BC?

Do you have a suggestion? Or should I add a new method? (but with what name?)
Also, since the library is not stable yet, I think ppl using this should expect frequent BC breaks and have their dependencies setup accordingly (ie ^0.2 will not automatically update to 0.3, unlike with stable versions)

Choose a reason for hiding this comment

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

Yeah we can introduce BC but I would like t know if we can maintain a retro-compatibility with this feature :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Im not sure I understand. :(

We can avoid the BC break by not changing this method signature, and instead add a new method with the new signature.
Does this answer your question?
In this case though, I'm not sure what name to give that method. Suggestions are welcome.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a fixup command adding a new method instead of changing the current method signature.

This avoids the BC break. We can also deprecate the old method and later recreate it with the new signature, for the sake of having a nice method name.

This provides better typing
@hgraca hgraca force-pushed the feat/replace_array_for_expansion branch from 70eb7a2 to fb43431 Compare August 26, 2023 21:07
@hgraca hgraca changed the title [BC_BREAK] Replace array for string ... Replace array for string ... Sep 23, 2023
@hgraca hgraca changed the title Replace array for string ... Create alternative to namespaceMatchesOneOfTheseNamespaces using splat instead of array Sep 23, 2023
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.

3 participants