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.

Sign up for GitHub

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

Jump to bottom

Fix #5578 namespaces/use placement #5579

Merged
merged 1 commit into from
Mar 4, 2023
Merged

Fix #5578 namespaces/use placement #5579

merged 1 commit into from
Mar 4, 2023

Conversation

rossluk
Copy link
Contributor

@rossluk rossluk commented Feb 28, 2023 •
edited
Loading

Fix "Fix Import" namespaces/uses placement in multiple php open tag files, when first line of php file is not empty or it is a block element, after declare statements in different scope

@mbien mbien added the PHP [ci] enable extra PHP tests (php/php.editor) label Feb 28, 2023
@mbien mbien linked an issue Feb 28, 2023 that may be closed by this pull request
PHP Fix Imports issues with namespace/use placing #5578
Closed
@apache apache locked and limited conversation to collaborators Feb 28, 2023
@apache apache unlocked this conversation Feb 28, 2023
@mbien mbien requested review from junichi11 and KacerCZ February 28, 2023 16:31
lbownik
lbownik reviewed Feb 28, 2023
View reviewed changes
php/php.editor/src/org/netbeans/modules/php/editor/actions/FixUsesPerformer.java
} catch (BadLocationException ex) {
Exceptions.printStackTrace(ex);
}
return false;
Copy link
Contributor

Choose a reason for hiding this comment

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

would return in catch block read better?

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 am not a java developer, if it would be better i am willing to make changes, but just checked few files with similar logic and there is usually return outside

@junichi11
Copy link
Member

Could you add unit tests?

@junichi11 junichi11 requested a review from tmysik February 28, 2023 23:08
tmysik
tmysik requested changes Mar 1, 2023
View reviewed changes
Copy link
Member

@tmysik tmysik left a comment

Choose a reason for hiding this comment

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

Please, add unit tests for your use case. Thank you.

@rossluk rossluk force-pushed the php/fix-uses-performer-fix branch from e74d654 to 37518bd Compare March 1, 2023 13:19
@rossluk
Copy link
Contributor Author

rossluk commented Mar 1, 2023

Added tests and fixes.

@rossluk
Copy link
Contributor Author

rossluk commented Mar 1, 2023

I've found more issues related to use placement, will do force push when have it fixed

@rossluk rossluk force-pushed the php/fix-uses-performer-fix branch from 37518bd to a43b2ab Compare March 1, 2023 23:56
@rossluk
Copy link
Contributor Author

rossluk commented Mar 2, 2023

Added new test case and refactored some code. There is still a problem with multiple global namespaces, but problem is not related to Fix Import and I suppose should be fixed by other PR.

@rossluk rossluk requested review from tmysik and junichi11 and removed request for KacerCZ, tmysik and junichi11 March 2, 2023 00:01
@rossluk rossluk force-pushed the php/fix-uses-performer-fix branch from a43b2ab to 243e8e5 Compare March 2, 2023 09:23
@junichi11
Copy link
Member

Please rename testNB5578 to testGH5578.
Please add issue number to the commit message.

@rossluk rossluk force-pushed the php/fix-uses-performer-fix branch from 243e8e5 to bcddec0 Compare March 2, 2023 12:14
tmysik
tmysik approved these changes Mar 2, 2023
View reviewed changes
Copy link
Member

@tmysik tmysik left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for your work!

@tmysik
Copy link
Member

tmysik commented Mar 2, 2023

@rossluk Please, wait for approval from @junichi11. Thank you.

@rossluk rossluk force-pushed the php/fix-uses-performer-fix branch from bcddec0 to bb46132 Compare March 3, 2023 11:34
@rossluk rossluk force-pushed the php/fix-uses-performer-fix branch from bb46132 to fb06369 Compare March 3, 2023 11:36
junichi11
junichi11 approved these changes Mar 4, 2023
View reviewed changes
Copy link
Member

@junichi11 junichi11 left a comment

Choose a reason for hiding this comment

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

Thank you for your work.

@junichi11 junichi11 merged commit d4944b7 into apache:master Mar 4, 2023
@rossluk rossluk deleted the php/fix-uses-performer-fix branch March 4, 2023 11:55
@junichi11 junichi11 added this to the NB18 milestone Jul 4, 2023
@junichi11 junichi11 mentioned this pull request Jul 5, 2023
Merged
junichi11 added a commit that referenced this pull request Jul 5, 2023
@junichi11
Revert "Merge pull request #5579 from rossluk/php/fix-uses-performer-…
c01cbbd 
…fix"

This reverts commit d4944b7, reversing
changes made to d090d70.
@junichi11 junichi11 mentioned this pull request Jul 11, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lbownik lbownik lbownik left review comments

@tmysik tmysik tmysik approved these changes

@junichi11 junichi11 junichi11 approved these changes

Assignees
No one assigned
Labels
PHP [ci] enable extra PHP tests (php/php.editor)
Projects
None yet
Milestone
NB18
Development

Successfully merging this pull request may close these issues.

PHP Fix Imports issues with namespace/use placing
5 participants
@rossluk @junichi11 @tmysik @lbownik @mbien