Skip to content

Conversation

@zhukoff74
Copy link
Contributor

This is:

  • a bugfix
  • a new feature
  • refactoring
  • additional unit tests

Checklist:

  • Changes are covered by unit tests
    • Changes are covered by existing unit tests
    • New unit tests have been added
  • Code style is respected
  • Commit message explains why the change is made (see https://github.com/erlang/otp/wiki/Writing-good-commit-messages)
  • CHANGELOG.md contains a short summary of the change and a link to the pull request if applicable
  • Documentation is updated as necessary

Why this change is needed?

  • Added the createSolidFillStyle() method to eliminate duplication
  • Simplified the testAppliedStyleWithRange and testAppliedStyleSingleCell tests
  • Simplified the testNoChangeToActiveSheet test
  • Improved code readability

$sheet1->setCellValue('A1', "='Sheet 3'!A1");
$cell = 'A1';
$spreadsheet->setActiveSheetIndex(0);
self::assertEquals(0, $spreadsheet->getActiveSheetIndex());
Copy link
Collaborator

Choose a reason for hiding this comment

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

As long as you are cleaning up this test member anyhow, would you mind changing all the assertEquals to assertSame where possible (which I'm guessing is all of them). It's a stronger test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok

@oleibman
Copy link
Collaborator

oleibman commented Dec 3, 2025

Also, line 120 is the only place with expectException without expectExceptionMessage. Please add the latter, so that we confirm we are receiving the exception for the right reason.

@oleibman oleibman added this pull request to the merge queue Dec 4, 2025
Merged via the queue into PHPOffice:master with commit dde7fe1 Dec 4, 2025
14 checks passed
@oleibman
Copy link
Collaborator

oleibman commented Dec 4, 2025

Thank you for your contribution.

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants