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

[Feauture] Support string arguments to utcOffset method #1395

Merged
merged 3 commits into from Mar 10, 2021

Conversation

ognjenjevremovic
Copy link
Contributor

Closes: #1085

↪️ Pull Request

  • Extend utcOffset method with support for string argument representing the offset.
  • Provide the unit test(s) for the code change.

Code example

Allow UTC offset to be defined as a string. Valid formats are:

  • +HH:mm,
  • -HH:mm,
  • +HHmm
  • -HHmm

Example usage:

dayjs().utcOffset(`+08:00`, true)

✔️ PR Todo

  • Added/updated unit tests for this change
  • Included links to related issues/PRs.

📝 Additional notes

After the change I ensured that:

  • all tests are passing, by running npm run test && npm run test-tz,
  • formating / code style is consistent, by running npm run lint*.

There are 2 failing unit tests present on the dev branch and as such are present in this fix as well, however they are not affected by this issue / code introduced.

Ognjen Jevremovic added 2 commits March 1, 2021 13:54
Provide support string support to utcOffset method. Valid string values
formats for utcOffset method are +HH:mm, -HH:mm, +HHmm and -HHmm.

✅ Closes: iamkun#1085
Extend the current suite of utc unit tests by providing unit tests for
the utcOffset method with string arguments.

✅ Closes: iamkun#1085
src/constant.js Outdated Show resolved Hide resolved
UTC plugin constants should be self-contained from within the plugin
package.

✅ Closes: iamkun#1395
export const REGEX_OFFSET_HOURS_MINUTES_FORMAT = /([+-]|\d\d)/g

function offsetFromString(value = '') {
const offset = value.match(REGEX_VALID_OFFSET_FORMAT)
Copy link
Owner

Choose a reason for hiding this comment

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

can we skip these three lines, and put everything to REGEX_OFFSET_HOURS_MINUTES_FORMAT regex?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure I understood you well.
Can you provide more info on the changes you would like regarding this code block?

Do you want the empty line removed and have the offsetFromString method just below the REGEX_OFFSET_HOURS_MINUTES_FORMAT, with no blank space in between?

Copy link
Owner

Choose a reason for hiding this comment

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

it seems REGEX_VALID_OFFSET_FORMAT and REGEX_OFFSET_HOURS_MINUTES_FORMAT is doing the same thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @iamkun the two provided regexes have some slight differences, but serve different purposes.

REGEX_VALID_OFFSET_FORMAT is used to test the provided string value format.
If the method argument is not prefixed with - or + symbol the method exists early with null value. That's the only purpose of this regex.

REGEX_OFFSET_HOURS_MINUTES_FORMAT is used to split the output of the first regex into symbol, hours, minutes variables. We need to split the string in order to provide the necessary calculations.
This could've been perhaps achieved using the String.prototype.slice method instead, but would require multiple calls (3 calls to be precise in order to extract symbol, hours and minutes values) - regex might be faster in this case.

We could merge these two together but the method logic should be updated to reflect this change. It would break SR principle (more of a personal preference) and might be more error prone.
I am able to provide the requested changes, just wanted to give you a brief overview of the design choices behind 2 regex pattern 🙂 .

Copy link
Owner

Choose a reason for hiding this comment

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

that make sense, thanks

@iamkun iamkun merged commit 656127c into iamkun:dev Mar 10, 2021
@aloisklink aloisklink mentioned this pull request Apr 23, 2021
iamkun pushed a commit that referenced this pull request May 26, 2021
## [1.10.5](v1.10.4...v1.10.5) (2021-05-26)

### Bug Fixes

* add meridiem in ar locales ([#1375](#1375)) ([319f616](319f616))
* Added Zulu support to customParseFormat ([#1359](#1359)) ([1138a3f](1138a3f))
* fix Bengali [bn] locale monthsShort error ([a0e6c0c](a0e6c0c))
* fix missing types for ArraySupport plugin ([#1401](#1401)) ([b1abdc4](b1abdc4))
* fix Ukrainian [uk] locale ([#1463](#1463)) ([0fdac93](0fdac93))
* hotfix for `Duration` types ([#1357](#1357)) ([855b7b3](855b7b3)), closes [#1354](#1354)
* timezone plugin DST error ([#1352](#1352)) ([71bed15](71bed15))
* Update duration plugin change string to number ([#1394](#1394)) ([e1546d1](e1546d1))
* update Duration plugin to support no-argument ([#1400](#1400)) ([8d9a5ae](8d9a5ae))
* Update Finnish [fi] locale to set yearStart  ([#1378](#1378)) ([f3370bd](f3370bd))
* update Russian [ru] locale meridiem and unit tests ([#1403](#1403)) ([f10f39d](f10f39d))
* update Russian [ru] locale yearStart config  ([#1372](#1372)) ([5052515](5052515))
* update Slovenian [sl] locale to set correct ordinal  ([#1386](#1386)) ([cb4f746](cb4f746))
* update Spanish [es] locale to change month names to lowercase ([#1414](#1414)) ([9c20e77](9c20e77))
* update Swedish [sv] locale to set correct yearStart ([#1385](#1385)) ([66c5935](66c5935))
* update UTC plugin to support string argument like +HH:mm ([#1395](#1395)) ([656127c](656127c))
@iamkun
Copy link
Owner

iamkun commented May 26, 2021

🎉 This PR is included in version 1.10.5 🎉

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.10.5](iamkun/dayjs@v1.10.4...v1.10.5) (2021-05-26)

### Bug Fixes

* add meridiem in ar locales ([#1375](iamkun/dayjs#1375)) ([319f616](iamkun/dayjs@319f616))
* Added Zulu support to customParseFormat ([#1359](iamkun/dayjs#1359)) ([1138a3f](iamkun/dayjs@1138a3f))
* fix Bengali [bn] locale monthsShort error ([a0e6c0c](iamkun/dayjs@a0e6c0c))
* fix missing types for ArraySupport plugin ([#1401](iamkun/dayjs#1401)) ([b1abdc4](iamkun/dayjs@b1abdc4))
* fix Ukrainian [uk] locale ([#1463](iamkun/dayjs#1463)) ([0fdac93](iamkun/dayjs@0fdac93))
* hotfix for `Duration` types ([#1357](iamkun/dayjs#1357)) ([855b7b3](iamkun/dayjs@855b7b3)), closes [#1354](iamkun/dayjs#1354)
* timezone plugin DST error ([#1352](iamkun/dayjs#1352)) ([71bed15](iamkun/dayjs@71bed15))
* Update duration plugin change string to number ([#1394](iamkun/dayjs#1394)) ([e1546d1](iamkun/dayjs@e1546d1))
* update Duration plugin to support no-argument ([#1400](iamkun/dayjs#1400)) ([8d9a5ae](iamkun/dayjs@8d9a5ae))
* Update Finnish [fi] locale to set yearStart  ([#1378](iamkun/dayjs#1378)) ([f3370bd](iamkun/dayjs@f3370bd))
* update Russian [ru] locale meridiem and unit tests ([#1403](iamkun/dayjs#1403)) ([f10f39d](iamkun/dayjs@f10f39d))
* update Russian [ru] locale yearStart config  ([#1372](iamkun/dayjs#1372)) ([5052515](iamkun/dayjs@5052515))
* update Slovenian [sl] locale to set correct ordinal  ([#1386](iamkun/dayjs#1386)) ([cb4f746](iamkun/dayjs@cb4f746))
* update Spanish [es] locale to change month names to lowercase ([#1414](iamkun/dayjs#1414)) ([9c20e77](iamkun/dayjs@9c20e77))
* update Swedish [sv] locale to set correct yearStart ([#1385](iamkun/dayjs#1385)) ([66c5935](iamkun/dayjs@66c5935))
* update UTC plugin to support string argument like +HH:mm ([#1395](iamkun/dayjs#1395)) ([656127c](iamkun/dayjs@656127c))
andrewhood125ruhuc added a commit to andrewhood125ruhuc/SidRH2 that referenced this pull request May 10, 2022
## [1.10.5](iamkun/dayjs@v1.10.4...v1.10.5) (2021-05-26)

### Bug Fixes

* add meridiem in ar locales ([#1375](iamkun/dayjs#1375)) ([319f616](iamkun/dayjs@319f616))
* Added Zulu support to customParseFormat ([#1359](iamkun/dayjs#1359)) ([1138a3f](iamkun/dayjs@1138a3f))
* fix Bengali [bn] locale monthsShort error ([a0e6c0c](iamkun/dayjs@a0e6c0c))
* fix missing types for ArraySupport plugin ([#1401](iamkun/dayjs#1401)) ([b1abdc4](iamkun/dayjs@b1abdc4))
* fix Ukrainian [uk] locale ([#1463](iamkun/dayjs#1463)) ([0fdac93](iamkun/dayjs@0fdac93))
* hotfix for `Duration` types ([#1357](iamkun/dayjs#1357)) ([855b7b3](iamkun/dayjs@855b7b3)), closes [#1354](iamkun/dayjs#1354)
* timezone plugin DST error ([#1352](iamkun/dayjs#1352)) ([71bed15](iamkun/dayjs@71bed15))
* Update duration plugin change string to number ([#1394](iamkun/dayjs#1394)) ([e1546d1](iamkun/dayjs@e1546d1))
* update Duration plugin to support no-argument ([#1400](iamkun/dayjs#1400)) ([8d9a5ae](iamkun/dayjs@8d9a5ae))
* Update Finnish [fi] locale to set yearStart  ([#1378](iamkun/dayjs#1378)) ([f3370bd](iamkun/dayjs@f3370bd))
* update Russian [ru] locale meridiem and unit tests ([#1403](iamkun/dayjs#1403)) ([f10f39d](iamkun/dayjs@f10f39d))
* update Russian [ru] locale yearStart config  ([#1372](iamkun/dayjs#1372)) ([5052515](iamkun/dayjs@5052515))
* update Slovenian [sl] locale to set correct ordinal  ([#1386](iamkun/dayjs#1386)) ([cb4f746](iamkun/dayjs@cb4f746))
* update Spanish [es] locale to change month names to lowercase ([#1414](iamkun/dayjs#1414)) ([9c20e77](iamkun/dayjs@9c20e77))
* update Swedish [sv] locale to set correct yearStart ([#1385](iamkun/dayjs#1385)) ([66c5935](iamkun/dayjs@66c5935))
* update UTC plugin to support string argument like +HH:mm ([#1395](iamkun/dayjs#1395)) ([656127c](iamkun/dayjs@656127c))
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.

[Feature Request] support utcOffset("+08:00")
2 participants