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

Don't remove existing uses if they are the same as the created string for use statements #6179

Merged
merged 1 commit into from
Jul 12, 2023

Conversation

junichi11
Copy link
Member

@junichi11 junichi11 added the PHP [ci] enable extra PHP tests (php/php.editor) label Jul 11, 2023
@junichi11 junichi11 added this to the NB19 milestone Jul 11, 2023
@junichi11
Copy link
Member Author

junichi11 commented Jul 11, 2023

I've fixed #5681 again.
I didn't fix the "multiple default namespaces" issue because I didn't think it's important.
I don't think many people use the following case.

<?php
namespace {
    class DefaultA {}
}

namespace MyNamespace {
    class MyClass {}
}

namespace {
    class DefaultB {}
}

I'm not sure the reason why a user would like to separate it.
Usually, it should be used the following, I think.

<?php
namespace {
    class DefaultA {}
    class DefaultB {}
}

namespace MyNamespace {
    class MyClass {}
}

I hope that the regressions are resolved in NB19.

@junichi11 junichi11 requested a review from tmysik July 12, 2023 00:50
@tmysik
Copy link
Member

tmysik commented Jul 12, 2023

@junichi11 I agree, it makes sense to me. We can always wait for a new report.

… for use statements

- apache#5681
- Avoid changing the file state if the created string is the same as the
  existing use statements
@junichi11 junichi11 merged commit f6df5f3 into apache:master Jul 12, 2023
34 checks passed
@junichi11 junichi11 deleted the php-gh-5681-reimplement branch July 12, 2023 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PHP [ci] enable extra PHP tests (php/php.editor)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants