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

Remove Psalm job #50

Merged
merged 1 commit into from
Dec 10, 2024
Merged

Remove Psalm job #50

merged 1 commit into from
Dec 10, 2024

Conversation

greg0ire
Copy link
Member

@greg0ire greg0ire commented Oct 8, 2024

We have decided to stop running two static analysers on our repositories, because of the extra maintenance it causes to us.

@stof
Copy link
Member

stof commented Oct 8, 2024

If phpstan is the tool being kept, all repos should first migrate their prefixed annotations to @phpstan* instead of @psalm-* because both tools are silently ignoring invalid annotations using the prefix of the other tool instead of reporting errors (because maybe they are actually valid in the other tool). Projects need to use the prefix for a tool they actually use to have a reliable CI.

@greg0ire
Copy link
Member Author

greg0ire commented Oct 8, 2024

We think that's fine, and we'll test that with a search and replace of psalm- before merging this.

@stof
Copy link
Member

stof commented Oct 8, 2024

@greg0ire I'm not opposed to remove Psalm at all. I just hinted that this replacement should be done in all projects (ideally before they stop using psalm)

@greg0ire
Copy link
Member Author

greg0ire commented Oct 8, 2024

Yeah, thanks for pointing that out, it made us discuss it more in depth, and come to the conclusion that it would be nicer to try the change before merging this. This will be released as a major version, and projects that want to keep using Psalm can copy paste the code I'm removing.

Copy link
Member

@SenseException SenseException left a comment

Choose a reason for hiding this comment

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

I too use both tools in my OS projects and always got covered by one of them when the other put a blind eye to an issue. If every Doctrine project is okay with dropping psalm, we don't need a split in two separate workflows, which would be an alternative.

Does it matter if the replacements of psalm-annotations happen after a merge? I believed that these annotations were compatible to each of the two tools.

@SenseException
Copy link
Member

Okay, nevermind. I just saw #51 😅

@ostrolucky
Copy link
Member

Where was the discussion about dropping Psalm?

@stof
Copy link
Member

stof commented Oct 8, 2024

@SenseException valid annotations are compatible with both tools. But reporting of invalid annotations is different, and you want invalid annotations in your own code to be reported by the tool you use.

@ostrolucky probably at the gathering of part of the Doctrine team taking place these days.

@greg0ire
Copy link
Member Author

greg0ire commented Oct 9, 2024

@stof you're correct, it's a discussion we had with @derrabus @alcaeus @GromNaN @beberlei @morozov . The reasons invoked were:

  • additional maintenance burden incurred by having to fight 2 tools instead of one
  • concerns about Psalm itself (how we sometimes have to wait for it to be compatible with newest versions of our dependencies)

@derrabus
Copy link
Member

derrabus commented Oct 9, 2024

We need a rebase here. 🙂

@derrabus derrabus marked this pull request as draft October 14, 2024 13:10
@derrabus
Copy link
Member

Converted to draft for the time being. Out projects still rely on the Psalm job being present, so let's sort that out first.

We have decided to stop running two static analysers on our repositories,
because of the extra maintenance it causes to us.
@greg0ire greg0ire marked this pull request as ready for review December 10, 2024 07:45
@greg0ire
Copy link
Member Author

@derrabus I believe that's addressed now, for the most part.

@greg0ire greg0ire merged commit 43ddb9b into doctrine:main Dec 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants