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

PHP: Implemented correct return type guessing for a function that returns an array #6697

Conversation

troizet
Copy link
Collaborator

@troizet troizet commented Nov 14, 2023

Code example:

function testArrayReturnType()
{
    return [1, 2];
}


function testArrayReturnTypeWithUnionType()
{
    if (true) {
        return 'string';
    }
    return [1, 2];
}

testArrayReturnType();
testArrayReturnTypeWithUnionType();

Navigator

before after
navigator_before navigator_after

Documentation tooltip

before after
documentation_before_1 documentsation_after
documentation_before_2 documentation_after_2

phpDoc generation

before after
phpdoc_before phpdoc_after
phpdoc_before_2 phpdoc_after_2

Code example:

class Foo {
    public function myFoo(){
        return [1, 2];
    }
}

class Bar extends Foo {

}

Overriding method

before after
overriding_before overriding_after

@mbien mbien added the PHP [ci] enable extra PHP tests (php/php.editor) label Nov 15, 2023
@apache apache locked and limited conversation to collaborators Nov 15, 2023
@apache apache unlocked this conversation Nov 15, 2023
@junichi11
Copy link
Member

@troizet Could you change the target branch to php-nb21-features to avoid conflict? I'll merge that branch into the master after NB20 is released.

@mbien
Copy link
Member

mbien commented Nov 16, 2023

@troizet Could you change the target branch to php-nb21-features to avoid conflict? I'll merge that branch into the master after NB20 is released.

@junichi11 it would be better to not use feature branches since they cause everything to be built and tested twice. The NB 20 release should not be far away, we are at the last rc most likely, so conflicts should be hopefully unlikely now.

@junichi11
Copy link
Member

There are conflicts between this change and the php-nb21-features branch.

@mbien
Copy link
Member

mbien commented Nov 16, 2023

Ah I see, I think I misunderstood the plan.

@troizet troizet force-pushed the php_function_guessing_array_return_type branch from 74b5e54 to bfc5ccd Compare November 16, 2023 14:40
@troizet troizet changed the base branch from master to php-nb21-features November 16, 2023 14:41
@troizet
Copy link
Collaborator Author

troizet commented Nov 16, 2023

@junichi11 Made a rebase to the php-nb21-features branch.

@mbien
Copy link
Member

mbien commented Nov 16, 2023

ok, I understood it correctly after all. So the problem with branches in the main repo is that CI will think it is a main branch and run all tests (and that is a lot of tests) every time the branch updates. This comes additionally to the tests which run in the PR.

you can check it here: https://github.com/apache/netbeans/actions?query=branch%3Aphp-nb21-features

the second problem is that it will add about 800mb per branch to the cache: https://github.com/apache/netbeans/actions/caches, and we have to stay below 10gb if possible.

So if we really want to use branches beyond master and delivery in the main repo, we have to make some changes how things currently work in CI.

edit: going to figure something out
edit2: #6710

@junichi11
Copy link
Member

junichi11 commented Nov 16, 2023

ok, I understood it correctly after all. So the problem with branches in the main repo is that CI will think it is a main branch and run all tests (and that is a lot of tests) every time the branch updates. This comes additionally to the tests which run in the PR.

Does it mean that all test will be run regardless of labels?
Ah, I see now, when PRs are created and when PRs are merged/branches are updated.

the second problem is that it will add about 800mb per branch to the cache: https://github.com/apache/netbeans/actions/caches, and we have to stay below 10gb if possible.

Should we remove caches of the feature branch manually at the moment?

So if we really want to use branches beyond master and delivery in the main repo, we have to make some changes how things currently work in CI.

I would like to use the feature branch with the following cases.

  • When supporting specific PHP version
  • When we have huge changes during the release phase (Maybe, it's risky to merge them into the master branch. If we find a critical issue and fix it, conflicts might occur between the master and the delivery, I think.)

@junichi11 junichi11 added this to the NB21 milestone Nov 16, 2023
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.

@mbien
Copy link
Member

mbien commented Nov 17, 2023

Ah, I see now, when PRs are created and when PRs are merged/branches are updated.

right. Since a merge to master for example will run all tests. Any branch of the main repo behaves like a primary branch right now - since we typically don't use extra branches (except for cnd which is work in progress), everything happens in clones until its ready.

Should we remove caches of the feature branch manually at the moment?

no need atm. We are still good cache size wise. It would automatically remove the least used cache first if it has storage issues.

I would like to use the feature branch with the following cases.

lets move the discussion to #6710 so that we don't use this PR here for off-topic discussions.

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.

Thanks a lot!

@junichi11
Copy link
Member

Thank you, guys! Merging.

@junichi11 junichi11 merged commit 81162ff into apache:php-nb21-features Nov 18, 2023
34 checks passed
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.

4 participants