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 patrol #206

Merged
merged 25 commits into from
Nov 27, 2020
Merged

Psalm patrol #206

merged 25 commits into from
Nov 27, 2020

Conversation

DanielBadura
Copy link
Contributor

No description provided.

@DanielBadura
Copy link
Contributor Author

Failing tests are not connected to the changes made but connected to psalm. Psalm requires https://github.com/composer/package-versions-deprecated which replaces https://github.com/Ocramius/PackageVersions. This is causing a missing dependency because the original one does not get installed anymore. Im not sure how to solve this and if this should be part of this PR.

Anyhow should package replacing via composer be part of this package knowledge?

@Ocramius
Copy link
Collaborator

Ocramius commented Oct 3, 2020

@DanielBadura wouldn't worry too much about it: I think the CI pipeline needs a bit of an overhaul before moving forward.

Specifically, in order to maintain things more efficiently, we need:

  • composer.lock that dependabot updates regularly (so at least we get relatively reproducible builds)
  • github actions
  • restricting to PHP 7.4

This patch should mostly be fine as-is

@DanielBadura
Copy link
Contributor Author

https://www.youtube.com/watch?v=DzUCC9pfoNw

Are workflows already enabled?
Psalm level 1 is not completed yet but i would now update the baseline with the missing fixes.

@DanielBadura DanielBadura marked this pull request as ready for review October 3, 2020 13:35
@DanielBadura
Copy link
Contributor Author

Now it should be ready for review @Ocramius 👍

@DanielBadura
Copy link
Contributor Author

Rebased to get the composer.lock here to update it accordingly.
Just as an information: With this patch we loose the 100% coverage due to the new line in JsonLoader here: https://github.com/maglnet/ComposerRequireChecker/pull/206/files#diff-05424265c13240e6c7092781ce770688R38

I could not figure out how to test this case since is_readable is called before. But removing the is_readable in favor for the false check of file_get_contents will crash some tests so i thing its still needed.

@Ocramius
Copy link
Collaborator

Ocramius commented Oct 4, 2020

@DanielBadura about that last bit, I'm wondering if we can replace it with an assertion? webmozart/assert makes those cases accessible, and roave/infection-static-analysis-plugin can then squish out mutants that aren't testable

@DanielBadura
Copy link
Contributor Author

@Ocramius im not quite sure if i understood you correctly. I pushed some changes. With this we dont loose the file_get_contents false code path. Or do you mean with infection we dont need the 100% coverage?

@Ocramius
Copy link
Collaborator

Let's re-discuss this one after a rebase on top of #210 (and discussion there first!)

@DanielBadura
Copy link
Contributor Author

Since #210 is merged now i will rebase this PR the next days

@DanielBadura
Copy link
Contributor Author

So this is rebased now. Still getting the error mentioned before:

Failing tests are not connected to the changes made but connected to psalm. Psalm requires https://github.com/composer/package-versions-deprecated which replaces https://github.com/Ocramius/PackageVersions. This is causing a missing dependency because the original one does not get installed anymore. Im not sure how to solve this and if this should be part of this PR.

Anyhow should package replacing via composer be part of this package knowledge?

As this is targeting v3.0.0 now it would be feasible to drop composer 1 support and with it get rid of PackageVersion and get the package version via composer 2 api. WDYT @Ocramius ?

And if so - extra PR for dropping composer v1 or smash it into this one? :D

@Ocramius
Copy link
Collaborator

As this is targeting v3.0.0 now it would be feasible to drop composer 1 support and with it get rid of PackageVersion and get the package version via composer 2 api. WDYT @Ocramius ?

Sounds good to me. Beware that we still need #218 for that to work correctly, since composer-plugin-api is a bit fiddly (and obviously does not behave like a normal package -.-).

I would suggest reviewing that, and then we do yet another (sorry!) rebase here to squish out ocramius/package-versions.

@DanielBadura
Copy link
Contributor Author

No problem at all :)
Waiting then for #218 & then we look again at this :)
Until then: Have a good weekend!

@Ocramius
Copy link
Collaborator

@DanielBadura this can now be rebased

@DanielBadura
Copy link
Contributor Author

Alright, thanks for the notification 👍

@DanielBadura
Copy link
Contributor Author

@Ocramius PR is rebased and PackageVersions is out in favor for the composer-runtime-api^2 as discussed above. So with this PR we drop composer 1 support.

This PR is from my side finished & ready :)

Copy link
Collaborator

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

Excellent, thanks @DanielBadura!

@Ocramius Ocramius self-assigned this Nov 27, 2020
@Ocramius Ocramius merged commit 8f52133 into maglnet:master Nov 27, 2020
@DanielBadura DanielBadura deleted the psalm-patrol branch November 30, 2020 15:55
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.

2 participants