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

Psalm integration #36

Merged
merged 4 commits into from
Jun 2, 2021
Merged

Conversation

nusphere
Copy link
Contributor

@nusphere nusphere commented Mar 29, 2021

Q A
QA yes

Summary
As decided during the Technical-Steering-Committee Meeting on August 3rd, 2020, Laminas wants to implement vimeo/psalm in all packages.

Required

  • Create a .psalm.xml in the project root
  • Copy and paste the contents from this psalm.xml
  • Run $ composer require vimeo/psalm
  • Run $ vendor/bin/psalm --set-baseline=psalm-baseline.xml
  • Add a composer script static-analysis with the command psalm --shepherd --stats
  • Remove Travis Support

Affected Issue:

Signed-off-by: Sebastian Hopfe <[email protected]>
Signed-off-by: Sebastian Hopfe <[email protected]>
@nusphere nusphere marked this pull request as draft March 29, 2021 15:33
@nusphere
Copy link
Contributor Author

nusphere commented Mar 29, 2021

@Ocramius - Should travis run here? i implemented a .travis file

@nusphere
Copy link
Contributor Author

Unfortunately, no travis is currently active in the repo, so I cannot test the pipeline.

Especially since you may want to switch to the GitHub Actions.

So I leave the .travis file in the merge as a template for a possible use of Travis or as a template for the GitHub actions.

@nusphere nusphere marked this pull request as ready for review March 30, 2021 09:26
@froschdesign
Copy link
Member

@nusphere

Especially since you may want to switch to the GitHub Actions.

We have to switch. See: https://github.com/laminas/technical-steering-committee/blob/eabf308b50272785d5d04c72fc3c64e0a7d7a2c9/meetings/minutes/2020-11-02-TSC-Minutes.md#github-actions

So I leave the .travis file in the merge…

Can be removed.

Signed-off-by: Sebastian Hopfe <[email protected]>
@nusphere
Copy link
Contributor Author

nusphere commented Mar 30, 2021

@froschdesign

on the one hand the skeleton is delivered without qa tooling, on the other hand these should be supported.
what applies to "require-dev"?

if psalm is used as a tooling and should be preinstalled (as required in the issue) laminas-test is also required.
Psalm as such belongs in the dev part of the composer.

or should all dependencies be removed (including laminas test) and then these errors ignored based on the baseline?

Signed-off-by: Sebastian Hopfe <[email protected]>
@nusphere
Copy link
Contributor Author

documentation adjusted and require dev's removed

@samsonasik samsonasik added this to the 1.2.0 milestone May 28, 2021
@@ -14,6 +14,6 @@ class Module
{
public function getConfig() : array
{
return include __DIR__ . '/../config/module.config.php';
return (array) include __DIR__ . '/../config/module.config.php';
Copy link
Member

Choose a reason for hiding this comment

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

I think we can do with :

/** @var array $config */
$config = include __DIR__ . '/../config/module.config.php';
return $config;

instead of (array) cast as the config/module.config.php is an array already.

@samsonasik samsonasik merged commit 2dde88e into laminas:1.2.x Jun 2, 2021
@samsonasik
Copy link
Member

@nusphere Thank you, I've handled infer return type at Module::getConfig() during merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Psalm integration
3 participants