Skip to content
This repository was archived by the owner on Apr 27, 2026. It is now read-only.

Sandbox::getExpirationTimestamp() return type fixup#82

Merged
ErikZigo merged 1 commit into
mainfrom
erik-PST-1512-part2
Apr 3, 2024
Merged

Sandbox::getExpirationTimestamp() return type fixup#82
ErikZigo merged 1 commit into
mainfrom
erik-PST-1512-part2

Conversation

@ErikZigo
Copy link
Copy Markdown
Contributor

@ErikZigo ErikZigo commented Apr 2, 2024

Sandbox::fromArray() se upravoval tady

e5efdb3#diff-13ebad36f392cf70c46dfa3df6cd7ee23946505c7e424e3b7d5ea0281ae98a72L87

takže bych řekl že nikdy nemůže nastat situace, že expirationAfterHours je null

@ErikZigo ErikZigo requested a review from pepamartinec April 2, 2024 15:32
Comment thread src/Sandbox.php
}

public function getExpirationAfterHours(): ?int
public function getExpirationAfterHours(): int
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

takže bych řekl že nikdy nemůže nastat situace, že expirationAfterHours je null

Muze, pokud udelas treba (new Sandbox())->getExpirationAfterHours(). Je to edge-case, ale technicky mozny (https://github.com/keboola/sandboxes/blob/36abe6e826be7bc8628972e5c671e16d1f03771d/src/Library/SandboxFactory.php#L22)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Jsem zil v domeni ze kdyz je ta property bez default hodnoty

private int $expirationAfterHours;

tak to pak hodi chybu kdyz jsou zaply strict types a neni default hodnota

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Z API se to vsude prevadi pres to Sandbox::fromArray() a tam se vsude misto null natavi 1

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ahaa, prehlidl jsme, ze ta property neni nullable. To neznamena, ze by tam ten edge-case neni, ale je tma uz davno 😄 a ta tvoje zmena to nijak nezhorsi 👍

@ErikZigo ErikZigo requested a review from pepamartinec April 3, 2024 07:11
@ErikZigo ErikZigo merged commit d0d4803 into main Apr 3, 2024
@ErikZigo ErikZigo deleted the erik-PST-1512-part2 branch April 3, 2024 07:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants