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

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

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

@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 ChristophWurst added this to TO REVIEW (max 4 PRs) in Christoph's Tasks via automation 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

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 removed the 0. Needs triage Pending check for reproducibility or if it fits our roadmap label Oct 14, 2020
@MorrisJobke MorrisJobke added the 3. to review Waiting for reviews label Oct 14, 2020
Christoph's Tasks automation moved this from TO REVIEW (max 4 PRs) to TO INTEGRATE 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
@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
@@ -68,14 +68,14 @@ public function search($search, array $shareTypes, $lookup, $limit, $offset) {
foreach ($this->pluginList[$type] as $plugin) {
/** @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
Open
@MorrisJobke MorrisJobke merged commit 9979b6d into master Oct 14, 2020
Christoph's Tasks automation moved this from TO INTEGRATE to DONE 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

@juliushaertl juliushaertl Awaiting requested review from juliushaertl

@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
No open projects
Christoph's Tasks
DONE
Milestone
Nextcloud 21
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@ChristophWurst @MorrisJobke @blizzz @kesselb