Skip to content

Conversation

@BackEndTea
Copy link
Contributor

This is:

  • refactoring

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?

Extra checks are currently needed on the result of the Escher::load method, as its not clear what the return type becomes.

By making this class generic, we allow to infer the return type from the constructor argument, for static analysis tools.

Additional get...OrThrow methods have been added, to throw more precise errors, rather than Fail due to a method call on null.

@oleibman
Copy link
Collaborator

Thank you for an interesting improvement. Please make it a little better by avoiding new uncovered code. Instead of:

    public function getDggContainerOrThrow(): Escher\DggContainer
    {
        if ($this->dggContainer !== null) {
            return $this->dggContainer;
        }

        throw new SpreadsheetException('dggContainer is unexpectedly null');
    }

Please try something like:

    public function getDggContainerOrThrow(): Escher\DggContainer
    {
            return $this->dggContainer ?? throw new SpreadsheetException('dggContainer is unexpectedly null');
    }

@BackEndTea
Copy link
Contributor Author

I considered that, but wanted to keep it in line with other get...OrThrow methods. I'll update these methods to the newer syntax

This removes the need to check if a method exists
@oleibman oleibman added this pull request to the merge queue Oct 24, 2025
@oleibman
Copy link
Collaborator

Thank you for making the change. You are correct that other routines could be similarly re-coded. It's all a question of priorities.

Merged via the queue into PHPOffice:master with commit 6494ddb Oct 24, 2025
14 checks passed
@oleibman
Copy link
Collaborator

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