Skip to content

Annotate IContainer so Psalm knows what resove and query return #23377

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
Merged
merged 2 commits into from
Oct 14, 2020
Merged

Annotate IContainer so Psalm knows what resove and query return #23377

merged 2 commits into from
Oct 14, 2020

Conversation

@ChristophWurst
Copy link
Member

@ChristophWurst ChristophWurst commented Oct 12, 2020 •
edited by MorrisJobke
Loading

@kesselb IIRC you wanted to add this some time ago. Were there any issues?

@ChristophWurst ChristophWurst added this to the Nextcloud 21 milestone Oct 12, 2020
@ChristophWurst ChristophWurst self-assigned this Oct 12, 2020
@ChristophWurst
Copy link
Member Author

@kesselb IIRC you wanted to add this some time ago. Were there any issues?

Bildschirmfoto von 2020-10-12 13-26-46

🙈

@MorrisJobke
Copy link
Member

The encryption app needs to move from those random names to just use the full namespace and then most of this is done.

@MorrisJobke
Copy link
Member

Also then most of the encryption app can be auto wired ;)

@ChristophWurst
Copy link
Member Author

The encryption app needs to move from those random names to just use the full namespace and then most of this is done.

While that is true it's still ok to use string that are not a FQCN. Let me see if we can teach Psalm that.

@ChristophWurst ChristophWurst added 0. Needs triage Pending check for reproducibility or if it fits our roadmap and removed 3. to review Waiting for reviews labels Oct 12, 2020
@ChristophWurst
Copy link
Member Author

The last commit will be removed. I'm just testing vimeo/psalm#4310.

@ChristophWurst ChristophWurst force-pushed the enhancement/psalm-annotate-icontainer branch from 25f0568 to 95ea74f Compare October 14, 2020 09:15
@ChristophWurst ChristophWurst mentioned this pull request Oct 14, 2020
Merged
@ChristophWurst @MorrisJobke
Annotate IContainer so Psalm knows what resove and query return
f9f20b6 
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@MorrisJobke MorrisJobke force-pushed the enhancement/psalm-annotate-icontainer branch from 95ea74f to f9f20b6 Compare October 14, 2020 10:29
@MorrisJobke
Copy link
Member

Rebased because other PR was merged.

@MorrisJobke
Copy link
Member

Only 4 errors left 🙌

@ChristophWurst
Copy link
Member Author

ChristophWurst commented Oct 14, 2020 •
edited
Loading

Only 4 errors left raised_hands

But in other news these are new errors detected because of the typing 🚀

@ChristophWurst
Copy link
Member Author

/backport f9f20b6 to stable20

@MorrisJobke
Copy link
Member

@ChristophWurst Ready for review, right?

@MorrisJobke MorrisJobke requested review from GretaD and blizzz October 14, 2020 11:43
@MorrisJobke MorrisJobke added 3. to review Waiting for reviews and removed 0. Needs triage Pending check for reproducibility or if it fits our roadmap labels Oct 14, 2020
@ChristophWurst ChristophWurst added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Oct 14, 2020
blizzz
blizzz reviewed Oct 14, 2020
View reviewed changes
@ChristophWurst ChristophWurst force-pushed the enhancement/psalm-annotate-icontainer branch from 2ffc565 to 0644a74 Compare October 14, 2020 13:16
@MorrisJobke

This comment has been minimized.

Sign in to view
@ChristophWurst
Fix type errors detected by Psalm
f464ef0 
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@ChristophWurst ChristophWurst force-pushed the enhancement/psalm-annotate-icontainer branch from 0644a74 to f464ef0 Compare October 14, 2020 13:40
@ChristophWurst
Copy link
Member Author

Psalm:

Fixed 🙏

ChristophWurst
ChristophWurst commented Oct 14, 2020
View reviewed changes
lib/private/Collaboration/Collaborators/Search.php
/** @var ISearchPlugin $searchPlugin */
$searchPlugin = $this->c->resolve($plugin);
$hasMoreResults |= $searchPlugin->search($search, $limit, $offset, $searchResult);
$hasMoreResults = $searchPlugin->search($search, $limit, $offset, $searchResult) || $hasMoreResults;
Copy link
Member Author

Choose a reason for hiding this comment

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

@blizzz hope the logic is unchanged. The search call should always happen no matter the value of $hasMoreResults, right? Asking because there might be semantics of the short-circuit evaluation of an or.

Copy link
Member

Choose a reason for hiding this comment

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

Ah right ... but yes - the search should happen in every plugin and then it is combined afterwards.

@faily-bot
Copy link

faily-bot bot commented Oct 14, 2020

🤖 beep boop beep 🤖

Here are the logs for the failed build:

Status of 34135: failure

acceptance-users

  • tests/acceptance/features/users.feature:103
Show full log 
  Scenario: change columns visibility                          # /drone/src/tests/acceptance/features/users.feature:103
    Given I act as Jane                                        # ActorContext::iActAs()
    And I am logged in as the admin                            # LoginPageContext::iAmLoggedInAsTheAdmin()
    And I open the User settings                               # SettingsMenuContext::iOpenTheUserSettings()
    And I open the settings                                    # AppSettingsContext::iOpenTheSettings()
      App settings could not be found
      The button to open the app settings could not be found (NoSuchElementException)
    And I see that the settings are opened                     # AppSettingsContext::iSeeThatTheSettingsAreOpened()
    When I toggle the showLanguages checkbox in the settings   # AppSettingsContext::iToggleTheCheckboxInTheSettingsTo()
    Then I see that the "Language" column is shown             # UsersSettingsContext::iSeeThatTheColumnIsShown()
    When I toggle the showLastLogin checkbox in the settings   # AppSettingsContext::iToggleTheCheckboxInTheSettingsTo()
    Then I see that the "Last login" column is shown           # UsersSettingsContext::iSeeThatTheColumnIsShown()
    When I toggle the showStoragePath checkbox in the settings # AppSettingsContext::iToggleTheCheckboxInTheSettingsTo()
    Then I see that the "Storage location" column is shown     # UsersSettingsContext::iSeeThatTheColumnIsShown()
    When I toggle the showUserBackend checkbox in the settings # AppSettingsContext::iToggleTheCheckboxInTheSettingsTo()
    Then I see that the "User backend" column is shown         # UsersSettingsContext::iSeeThatTheColumnIsShown()

@MorrisJobke MorrisJobke mentioned this pull request Oct 14, 2020
Closed
@MorrisJobke MorrisJobke merged commit 9979b6d into master Oct 14, 2020
@MorrisJobke MorrisJobke deleted the enhancement/psalm-annotate-icontainer branch October 14, 2020 15:04
@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Oct 14, 2020
Merged
@MorrisJobke MorrisJobke mentioned this pull request Nov 30, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MorrisJobke MorrisJobke MorrisJobke approved these changes

@blizzz blizzz blizzz approved these changes

@kesselb kesselb kesselb approved these changes

@juliusknorr juliusknorr Awaiting requested review from juliusknorr

@GretaD GretaD Awaiting requested review from GretaD

Assignees

@ChristophWurst ChristophWurst

Labels

4. to release Ready to be released and/or waiting for tests to finish enhancement technical debt

Projects

None yet

Milestone

Nextcloud 21

Development

Successfully merging this pull request may close these issues.

5 participants

@ChristophWurst @MorrisJobke @blizzz @kesselb