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

feat: new methods setDate / getDate #625

Merged
merged 2 commits into from
Sep 21, 2022

Conversation

flaviendelangle
Copy link
Collaborator

@flaviendelangle flaviendelangle commented Sep 12, 2022

Alternative names: getDayInMonth / setDayInMonth (can be confusing with getDaysInMonth)

@flaviendelangle flaviendelangle added the enhancement New feature or request label Sep 12, 2022
@flaviendelangle flaviendelangle self-assigned this Sep 12, 2022
@dmtrKovalenko
Copy link
Owner

Please make sure that CI passes

@codecov-commenter
Copy link

codecov-commenter commented Sep 13, 2022

Codecov Report

Merging #625 (6d5b8b6) into master (b39210e) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #625   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           16        16           
  Lines         1498      1536   +38     
  Branches       195       195           
=========================================
+ Hits          1498      1536   +38     
Impacted Files Coverage Δ
...kages/date-fns-jalali/src/date-fns-jalali-utils.ts 100.00% <100.00%> (ø)
packages/date-fns/src/date-fns-utils.ts 100.00% <100.00%> (ø)
packages/dayjs/src/dayjs-utils.ts 100.00% <100.00%> (ø)
packages/hijri/src/hijri-utils.ts 100.00% <100.00%> (ø)
packages/jalaali/src/jalaali-utils.ts 100.00% <100.00%> (ø)
packages/js-joda/src/js-joda-utils.ts 100.00% <100.00%> (ø)
packages/luxon/src/luxon-utils.ts 100.00% <100.00%> (ø)
packages/moment/src/moment-utils.ts 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@flaviendelangle
Copy link
Collaborator Author

I feel like Codecov is 1 commit behind
Yesterday I pushed 1 commit with 3-4 lines of code uncovered => no report, just the CI fail
This morning I pushed 1 commit to fix with 0 lines of code uncovered => report saying I have 3-4 lines of code uncovered
Just now I pushed 1 empty commit => report saying I have 0 lines of code uncovered

Comment on lines +200 to +201
getDate(value: TDate): number;
setDate(value: TDate, count: number): TDate;
Copy link
Owner

Choose a reason for hiding this comment

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

WDYT about making it getDay. I think it is more clear in terms of Date is the term for javascript date and may be confusing that it returns a number.

Copy link
Owner

Choose a reason for hiding this comment

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

Yes getDayInMonths or simply getDay would be fine. Everything else is great, so once you will done the rename – we can merge

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I based my naming on the javascript Date interface

new Date().getDay() => the day in the week
new Date().getDate() => the day in the month

But I don't have a strong opinion here
Libraries are not even coherent between them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@dmtrKovalenko dmtrKovalenko changed the title New methods setDate / getDate feat: new methods setDate / getDate Sep 21, 2022
@dmtrKovalenko dmtrKovalenko merged commit 8deea4a into dmtrKovalenko:master Sep 21, 2022
@flaviendelangle flaviendelangle deleted the getDate-setDate branch September 21, 2022 08:07
This was referenced Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants