Skip to content
This repository has been archived by the owner on Dec 1, 2022. It is now read-only.

Expose time utils. #571

Merged
merged 2 commits into from
Jun 30, 2021
Merged

Conversation

Shylock-Hg
Copy link
Contributor

Expose the time utils which used in cpp-client.

src/common/base/Status.h Outdated Show resolved Hide resolved
src/common/time/TimeUtils.cpp Outdated Show resolved Hide resolved
src/common/time/TimeUtils.cpp Outdated Show resolved Hide resolved
src/common/time/TimeUtils.cpp Outdated Show resolved Hide resolved
return dateTimeShift(dateTime, Timezone::getGlobalTimezone().utcOffsetSecs());
}

/*static*/ StatusOr<DateTime> TimeUtils::localDateTime() {
Copy link
Contributor

Choose a reason for hiding this comment

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

LocalDateTime () does not need to be exposed to the user, so StatusOr and Status does not need to be exposed to the cpp client

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Many method return status.

@@ -230,12 +233,12 @@ class StatusOr final {
// Return the associated `Status' if and only if it has one,
//
Status status() const & {
CHECK(hasStatus());
assert(hasStatus());
Copy link
Contributor

Choose a reason for hiding this comment

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

The Status no need to expose to the client, you can revert it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@Shylock-Hg Shylock-Hg requested review from Aiee, laura-ding and a team June 30, 2021 08:54
laura-ding
laura-ding previously approved these changes Jun 30, 2021
src/common/time/TimeConversion.cpp Outdated Show resolved Hide resolved
@CPWstatic CPWstatic merged commit a16f3fd into vesoft-inc:master Jun 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
ready-for-testing PR: ready for the CI test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants