-
Notifications
You must be signed in to change notification settings - Fork 659
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
aio-interface: Improve psalm #5369
Conversation
Perfect, the workflow works, it fails successfully ^^' PR ready |
I fear we cannot merge this with red ci. Can you run cs fix and commit the changes in a separate pr that builds on this branch? Also the changes to psalm seem to be unrelated? Can you put this into a separate pr please? |
I know...
I don't know, it is link with |
ok psalm done |
ok see #5370 for CS |
Signed-off-by: Jean-Yves <[email protected]>
Seems like static analysis is failing now :/ Can we increase the error level in a follow-up? |
In commit c20a8f8 The problem is that the workflow fails. This is due to some problems in the code. Ideally it would be good to switch to level 1. This avoids using variables of type |
That's why I used a TODO comment and added the stric composer script. |
Issue in code:
Command used: composer run psalm -- --monochrome --no-progress | grep ^ERROR | sed -e 's/^ERROR: //' | sort -u |
Ah I see. Can we for now switch back to errorlevel2 and then in follow-ups resolve the issues? |
I drop the commit |
I don't know if you've seen it yet, but I've started working on stalwart integration. I'm trying to apply strict rules in static analysis. https://github.com/docjyJ/stalwart-connector (I recently got into rust and really enjoyed it. I'm a bit of a neat freak, but I think it's important to avoid errors. 😅) |
Follow up #5368 |
I found that in psalm doc: https://psalm.dev/docs/manipulating_code/fixing/ |
Follow up already noted in #5368 Signed-off-by: Simon L. <[email protected]>
Signed-off-by: Simon L. <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks 😊
This sounds cool as an additonal command maybe? :) |
Also this looks very interesting! Cool that you started working on it 😊 |
I think this should be added globally to the Nextcloud style rule to maintain consistency. |
Thank you! If you have time, don't hesitate to give feedback. |
This is now released with v9.7.0 Beta. Testing and feedback is welcome! See https://github.com/nextcloud/all-in-one#how-to-switch-the-channel |
Fix warnings generated by phpstorm
psalm:strict
until type and unused class warnings are fixed (a lot277 other issues found.
)