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.

Sign up for GitHub

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

Jump to bottom

feat(DateTime) Provide a human-like months substract and add system #477

Merged
merged 1 commit into from
Jun 10, 2024
Merged

feat(DateTime) Provide a human-like months substract and add system #477

merged 1 commit into from
Jun 10, 2024

Conversation

veewee
Copy link
Collaborator

@veewee veewee commented Jun 7, 2024

As mentioned in

This PR introduces a way of adding and subsctracting months that is more in line with how you would do it as a human, rather than a computer.

I know you told me a few times:

The behaviour there is still the same, i personally think it is okay. its not broken. as there is no "standard" way of doing things here, every lib / lang does something that is completely different

However, I hope you consider taking this approach when it's just a "merge" button click away :)

@coveralls
Copy link

coveralls commented Jun 7, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9417866510

Details

  • 28 of 28 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.003%) to 97.27%
Totals Coverage Status
Change from base Build 9285294552: 0.003%
Covered Lines: 5023
Relevant Lines: 5164
💛 - Coveralls

@veewee veewee force-pushed the alternative-month-manipulation branch 3 times, most recently from a0ce49f to 932a6b3 Compare June 7, 2024 13:43
@coveralls
Copy link

coveralls commented Jun 7, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9418018792

Details

  • 32 of 32 (100.0%) changed or added relevant lines in 2 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.003%) to 97.27%
Files with Coverage Reduction New Missed Lines %
src/Psl/DateTime/Internal/high_resolution_time.php 2 90.48%
Totals Coverage Status
Change from base Build 9285294552: 0.003%
Covered Lines: 5023
Relevant Lines: 5164
💛 - Coveralls

@coveralls
Copy link

coveralls commented Jun 7, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9418010142

Details

  • 32 of 32 (100.0%) changed or added relevant lines in 2 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.003%) to 97.27%
Files with Coverage Reduction New Missed Lines %
src/Psl/DateTime/Internal/high_resolution_time.php 2 90.48%
Totals Coverage Status
Change from base Build 9285294552: 0.003%
Covered Lines: 5023
Relevant Lines: 5164
💛 - Coveralls

@coveralls
Copy link

coveralls commented Jun 7, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9418039594

Details

  • 28 of 28 (100.0%) changed or added relevant lines in 1 file are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.003%) to 97.27%
Files with Coverage Reduction New Missed Lines %
src/Psl/DateTime/Internal/high_resolution_time.php 2 90.48%
Totals Coverage Status
Change from base Build 9285294552: 0.003%
Covered Lines: 5023
Relevant Lines: 5164
💛 - Coveralls

@veewee
Copy link
Collaborator Author

veewee commented Jun 7, 2024

(The mutation tests nor failing windows unit test seem related to this PR)

@veewee veewee requested a review from azjezz June 7, 2024 13:50
azjezz
azjezz requested changes Jun 7, 2024
View reviewed changes
Copy link
Owner

@azjezz azjezz left a comment

Choose a reason for hiding this comment

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

LGTM! just nitpicking 😅

src/Psl/DateTime/DateTimeConvenienceMethodsTrait.php Outdated Show resolved Hide resolved
src/Psl/DateTime/DateTimeConvenienceMethodsTrait.php Outdated Show resolved Hide resolved
src/Psl/DateTime/DateTimeConvenienceMethodsTrait.php Outdated Show resolved Hide resolved
@veewee veewee force-pushed the alternative-month-manipulation branch 2 times, most recently from 80894ad to 5094253 Compare June 8, 2024 07:50
@coveralls
Copy link

coveralls commented Jun 8, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9427230862

Details

  • 30 of 30 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.004%) to 97.271%
Totals Coverage Status
Change from base Build 9285294552: 0.004%
Covered Lines: 5025
Relevant Lines: 5166
💛 - Coveralls

@coveralls
Copy link

coveralls commented Jun 8, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9427239203

Details

  • 30 of 30 (100.0%) changed or added relevant lines in 1 file are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.04%) to 97.232%
Files with Coverage Reduction New Missed Lines %
src/Psl/DateTime/Internal/high_resolution_time.php 2 80.95%
Totals Coverage Status
Change from base Build 9285294552: -0.04%
Covered Lines: 5023
Relevant Lines: 5166
💛 - Coveralls

@veewee veewee force-pushed the alternative-month-manipulation branch from 5094253 to 739e0e6 Compare June 9, 2024 16:19
@veewee veewee requested a review from azjezz June 9, 2024 16:21
@coveralls
Copy link

coveralls commented Jun 9, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9437924640

Details

  • 30 of 30 (100.0%) changed or added relevant lines in 1 file are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.04%) to 97.232%
Files with Coverage Reduction New Missed Lines %
src/Psl/DateTime/Internal/high_resolution_time.php 2 80.95%
Totals Coverage Status
Change from base Build 9285294552: -0.04%
Covered Lines: 5023
Relevant Lines: 5166
💛 - Coveralls

@veewee veewee added this to the 3.0.0 milestone Jun 10, 2024
@veewee veewee added Priority: Medium This issue may be useful, and needs some attention. Status: Accepted It's clear what the subject of the issue is about, and what the resolution should be. Type: Enhancement Most issues will probably ask for additions or changes. labels Jun 10, 2024
@veewee veewee merged commit ae3bf02 into azjezz:next Jun 10, 2024
12 of 14 checks passed
azjezz
azjezz reviewed Jun 10, 2024
View reviewed changes
src/Psl/DateTime/DateTimeConvenienceMethodsTrait.php
Comment on lines +445 to +446
$minus_years = Math\div($months, 12);
$months_left = $months - ($minus_years * 12);
Copy link
Owner

Choose a reason for hiding this comment

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

todo: 12 should be replaced with MONTHS_PER_YEAR ( just in case humanity decided to change the calender, so we have less work todo 😛 )

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

@azjezz azjezz azjezz approved these changes

Assignees
No one assigned
Labels
Priority: Medium This issue may be useful, and needs some attention. Status: Accepted It's clear what the subject of the issue is about, and what the resolution should be. Type: Enhancement Most issues will probably ask for additions or changes.
Projects
None yet
Milestone
3.0.0
Development

Successfully merging this pull request may close these issues.
3 participants
@veewee @coveralls @azjezz