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

Clean up redundant annotations and dead code in tests #3364

Merged
merged 5 commits into from
Nov 30, 2018

Conversation

BenMorel
Copy link
Contributor

Q A
Type improvement
BC Break no
Fixed issues N/A

Summary

This PR:

  • cleans up annotations that are:
    • useless because the variable does not exist: WriteTest
    • useless because the variable is assigned the return value of a properly documented function: BlobTest, DataAccessTest, MasterSlaveConnectionTest, ModifyLimitQueryTest, PortabilityTest
  • cleans up an annotation + a variable that are unused: TypeConversionTest

Copy link
Member

@morozov morozov left a comment

Choose a reason for hiding this comment

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

This look like a non-breaking change. Would it make sense to target it to master? If you want to work further on code cleanup, please consider raising the PHPStan level first. This way, we'll get more consistent and automated fixes.

@BenMorel BenMorel changed the base branch from develop to master November 29, 2018 17:49
@BenMorel
Copy link
Contributor Author

You're right, rebased on and retargeted to master. I'm too much focused on upcoming 3.0!

Can I be confident that master will always be merged into develop?
I'm more than happy to contribute to master, but want to be sure than my changes will end up in develop as well.

@morozov
Copy link
Member

morozov commented Nov 30, 2018

Can I be confident that master will always be merged into develop?

Yes. develop is based on master and is continuously rebased.

@morozov
Copy link
Member

morozov commented Nov 30, 2018

There seems to be more than that. If you search for a pattern like /\*\* @var \S+ \$ in the codebase on this branch, there will be 19 matches. By looking at the first 4, I can say that 3 of them can be removed:

  1. /** @var stdClass $serverInfo */
    $serverInfo = db2_server_info($this->conn);
  2. foreach ($table->getColumns() as $column) {
    /** @var Column $column */
  3. foreach ($diff->changedColumns as $columnDiff) {
    if ($this->onSchemaAlterTableChangeColumn($columnDiff, $diff, $columnSql)) {
    continue;
    }
    /** @var ColumnDiff $columnDiff */

I think we should address them all at once to create as little code-style noise as possible.

@BenMorel
Copy link
Contributor Author

Good point, I've removed a lot more! 👍

Copy link
Member

@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.

🚢

@Ocramius Ocramius self-assigned this Nov 30, 2018
@Ocramius Ocramius added this to the 3.0.0 milestone Nov 30, 2018
@Ocramius Ocramius merged commit 0ee865a into doctrine:master Nov 30, 2018
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 10, 2022
@morozov morozov modified the milestones: 4.0.0, 2.10.2 Aug 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants