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

Return null instead of false #2340

Merged
merged 8 commits into from May 28, 2021
Merged

Return null instead of false #2340

merged 8 commits into from May 28, 2021

Conversation

kylekatarnls
Copy link
Collaborator

Use stronger typing and null instead of false

@codecov-commenter
Copy link

codecov-commenter commented May 26, 2021

Codecov Report

Merging #2340 (1f38f65) into 3.x (c93e4f6) will increase coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@              Coverage Diff              @@
##                3.x     #2340      +/-   ##
=============================================
+ Coverage     99.95%   100.00%   +0.04%     
=============================================
  Files           882       886       +4     
  Lines          8525      9269     +744     
=============================================
+ Hits           8521      9269     +748     
+ Misses            4         0       -4     
Impacted Files Coverage Δ
src/Carbon/Carbon.php 100.00% <ø> (ø)
src/Carbon/CarbonImmutable.php 100.00% <ø> (ø)
src/Carbon/Cli/Invoker.php 100.00% <ø> (ø)
src/Carbon/Exceptions/NotACarbonClassException.php 100.00% <ø> (ø)
src/Carbon/Lang/aa_ET.php 100.00% <ø> (ø)
src/Carbon/Lang/am_ET.php 100.00% <ø> (ø)
src/Carbon/Lang/anp_IN.php 100.00% <ø> (ø)
src/Carbon/Lang/as_IN.php 100.00% <ø> (ø)
src/Carbon/Lang/ayc_PE.php 100.00% <ø> (ø)
src/Carbon/Lang/bhb_IN.php 100.00% <ø> (ø)
... and 310 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0fdfe5c...1f38f65. Read the comment docs.

@spawnia
Copy link
Contributor

spawnia commented May 27, 2021

I like where this is going, null is definitely more convenient then false, but why shouldn't methods such as create() always return a Carbon instance (or throw in case of invalid inputs)?

@kylekatarnls
Copy link
Collaborator Author

This is the planned progression: strict mode becomes true by default from 3.0.0 and disappears (to be always true) in 4.0.0, this means most of the method won't return null any longer (except make() or such which are very dedicated to that purpose).

Actually with the default strict mode already in 3.0.0 you can't get null from create()

@spawnia
Copy link
Contributor

spawnia commented May 27, 2021

Got it, that makes a lot of sense to me. Good to see this library turning away from PHP's error swallowing legacy swamp.

@kylekatarnls
Copy link
Collaborator Author

kylekatarnls commented May 27, 2021

This is hoping it will also evolve on PHP side because providing methods like createFromFormat(): Carbon|null while DateTime has createFromFormat(): DateTime|false is actually violating the Liskov Substitution Principle making Carbon a bit less compatible with its parent. 😞

So it will still need careful checking with dependent packages to validate we can keep it for the future stable release but I have good hope.

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

Successfully merging this pull request may close these issues.

None yet

3 participants