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:add plugin objectSupport #887

Merged
merged 14 commits into from
Apr 30, 2020
Merged

feat:add plugin objectSupport #887

merged 14 commits into from
Apr 30, 2020

Conversation

zerooverture
Copy link
Contributor

add/set/subtract support Object parameter
and constructor support Object parameter

The related unit tests have been added.

add/set/subtract support Object parameter
and constructor support Object parameter
@zerooverture zerooverture changed the title feat add plugin objectSupport feat:add plugin objectSupport Apr 27, 2020
@codecov
Copy link

codecov bot commented Apr 27, 2020

Codecov Report

Merging #887 into dev will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##               dev      #887   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          164       165    +1     
  Lines         1341      1385   +44     
  Branches       281       295   +14     
=========================================
+ Hits          1341      1385   +44     
Impacted Files Coverage Δ
src/plugin/objectSupport/index.js 100.00% <100.00%> (ø)

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 db642ac...83f3206. Read the comment docs.

oldParse.bind(this)(cfg)
}

proto.setObject = function (argument) {
Copy link
Owner

Choose a reason for hiding this comment

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

seems no need to put it on the proto, just a simple function will be better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you are right.

@iamkun
Copy link
Owner

iamkun commented Apr 28, 2020

Help us keeping 100% test coverage https://github.com/iamkun/dayjs/blob/dev/CONTRIBUTING.md

@zerooverture
Copy link
Contributor Author

Help us keeping 100% test coverage https://github.com/iamkun/dayjs/blob/dev/CONTRIBUTING.md
have already accomplished

declare module 'dayjs' {
interface Dayjs {
set(argument: object): Dayjs
add(argument?: object): Dayjs
Copy link
Owner

Choose a reason for hiding this comment

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

why a ? here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry, I was careless.processed.

sorry, I was careless
*/
const parseObjectArgument = (d, utc) => parseArrayArgument([
0,
d.y || d.year || d.years || 1970,
Copy link
Owner

Choose a reason for hiding this comment

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

check our this.$utils().p() prettyUnit to better handle this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Their role is different, d is input by the user
d => { years: 2010, months: 1, days: 14, hours: 15, minutes: 25, seconds: 50, milliseconds: 125 }
or
d => { y: 2010, M: 1, d: 14, h: 15, m: 25, s: 50, ms: 125 }
I don't have to judge units, just need to get the value of some units, just like the REGEX_PARSE in constant.js.
This code is similar to the part in index.js where parseDate handles strings.
const d = date.match(C.REGEX_PARSE) In the index.js 60 lines

Copy link
Owner

@iamkun iamkun Apr 29, 2020

Choose a reason for hiding this comment

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

https://github.com/iamkun/dayjs/blob/dev/src/plugin/duration/index.js#L42

Something like this to process all kinds of user input.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is that right? But it doesn't support date-> dayjs({date:1})

Copy link
Owner

Choose a reason for hiding this comment

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

1 you can get prettyUnit from this.$utils().p instead of important it directlly.

2 you may have to extend a new prettyUnit function to treat day and date key both mean day-of-the-month. like https://github.com/iamkun/dayjs/blob/dev/src/plugin/duration/index.js#L26

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is that all right?

Copy link
Owner

Choose a reason for hiding this comment

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

cool

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, seniors, let me learn a lot.
This is my first PR. Thanks♪(・ω・)ノ

Copy link
Owner

Choose a reason for hiding this comment

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

You are most welcome, cheers.

$d.millisecond || 0
]
if (utc) return new Date(Date.UTC(...arr))
return new Date(...arr)
Copy link
Owner

Choose a reason for hiding this comment

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

please avoid using this ...arr advanced es6 syntax, this will hugely increase the bundle size

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems much smaller now in packed


const oldParse = proto.parse
proto.parse = function (cfg) {
// console.log(cfg)
Copy link
Owner

Choose a reason for hiding this comment

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

no test code, pls.

return chain
}

const subtractObject = function (argument) {
Copy link
Owner

Choose a reason for hiding this comment

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

these three functions could be combined into one function with different arguments

return oldAdd.bind(this)(number, string)
}
const oldSubtract = proto.subtract
proto.subtract = function (number, string) {
Copy link
Owner

Choose a reason for hiding this comment

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

substract could be implemented as negative add, subtract { day: 1 } => add { day: 1 * -1 }, so that we could only care add and set logic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the three methods have been merged into one.

}

const oldSet = function (int, string) {
return this.clone().$set(string, int)
Copy link
Owner

Choose a reason for hiding this comment

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

why a copy-paste instead of proto.set

Copy link
Contributor Author

@zerooverture zerooverture Apr 30, 2020

Choose a reason for hiding this comment

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

here I've changed the set to have the same parameter location as add, otherwise you have to judge in callObject, which is inefficient and increases the package size.
or set and add can't share a callObject.

also, my attempt to use proto.set in oldSet will cause a dead loop because proto.set is redirected.
Using 'proto.$set' has the problem of the 'this' scope.

Copy link
Owner

Choose a reason for hiding this comment

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

Oh I see, why useing return proto.set will cause a dead loop?

Copy link
Contributor Author

@zerooverture zerooverture Apr 30, 2020

Choose a reason for hiding this comment

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

const oldSet = function (int, string) {
    return proto.set(string, int)
  }
...
proto.set = function (string, int) {
...
}

his code causes oldSet to enter the newly defined proto.set each time, So it creates a dead loop.

In the new code, I direct used a callback to solve this pointing problem.

@iamkun iamkun merged commit 52dfb13 into iamkun:dev Apr 30, 2020
iamkun pushed a commit that referenced this pull request May 14, 2020
## [1.8.27](v1.8.26...v1.8.27) (2020-05-14)

### Bug Fixes

* Add Kinyarwanda (rw) locale ([#903](#903)) ([f355235](f355235))
* Add plugin objectSupport ([#887](#887)) ([52dfb13](52dfb13))
* Add Turkmen (tk) locale ([#893](#893)) ([a9ca8dc](a9ca8dc))
* Fix CustomParseFormat plugin set locale error ([#896](#896)) ([8035c8a](8035c8a))
* Fix locale month function bug ([#908](#908)) ([bf347c3](bf347c3))
* Update CustomParseFormat plugin to support Array formats ([#906](#906)) ([97856c6](97856c6))
@iamkun
Copy link
Owner

iamkun commented May 14, 2020

🎉 This PR is included in version 1.8.27 🎉

The release is available on:

Your semantic-release bot 📦🚀

andrewhood125ruhuc added a commit to andrewhood125ruhuc/SidRH2 that referenced this pull request May 10, 2022
## [1.8.27](iamkun/dayjs@v1.8.26...v1.8.27) (2020-05-14)

### Bug Fixes

* Add Kinyarwanda (rw) locale ([#903](iamkun/dayjs#903)) ([f355235](iamkun/dayjs@f355235))
* Add plugin objectSupport ([#887](iamkun/dayjs#887)) ([52dfb13](iamkun/dayjs@52dfb13))
* Add Turkmen (tk) locale ([#893](iamkun/dayjs#893)) ([a9ca8dc](iamkun/dayjs@a9ca8dc))
* Fix CustomParseFormat plugin set locale error ([#896](iamkun/dayjs#896)) ([8035c8a](iamkun/dayjs@8035c8a))
* Fix locale month function bug ([#908](iamkun/dayjs#908)) ([bf347c3](iamkun/dayjs@bf347c3))
* Update CustomParseFormat plugin to support Array formats ([#906](iamkun/dayjs#906)) ([97856c6](iamkun/dayjs@97856c6))
andrewhood125ruhuc added a commit to andrewhood125ruhuc/SidRH2 that referenced this pull request May 10, 2022
## [1.8.27](iamkun/dayjs@v1.8.26...v1.8.27) (2020-05-14)

### Bug Fixes

* Add Kinyarwanda (rw) locale ([#903](iamkun/dayjs#903)) ([f355235](iamkun/dayjs@f355235))
* Add plugin objectSupport ([#887](iamkun/dayjs#887)) ([52dfb13](iamkun/dayjs@52dfb13))
* Add Turkmen (tk) locale ([#893](iamkun/dayjs#893)) ([a9ca8dc](iamkun/dayjs@a9ca8dc))
* Fix CustomParseFormat plugin set locale error ([#896](iamkun/dayjs#896)) ([8035c8a](iamkun/dayjs@8035c8a))
* Fix locale month function bug ([#908](iamkun/dayjs#908)) ([bf347c3](iamkun/dayjs@bf347c3))
* Update CustomParseFormat plugin to support Array formats ([#906](iamkun/dayjs#906)) ([97856c6](iamkun/dayjs@97856c6))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants