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

Add timestamp to events #1832

Merged
merged 11 commits into from
Aug 9, 2019
Merged

Add timestamp to events #1832

merged 11 commits into from
Aug 9, 2019

Conversation

windmemory
Copy link
Member

Refer to #1829

huan
huan approved these changes Aug 8, 2019
View reviewed changes
Copy link
Member

@huan huan left a comment

Choose a reason for hiding this comment

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

I have some minor thoughts on this PR, please follow and let's see what we should think next.

src/user/room.ts Outdated Show resolved Hide resolved
src/wechaty.ts Outdated Show resolved Hide resolved
src/wechaty.ts Outdated Show resolved Hide resolved
huan
huan requested changes Aug 8, 2019
View reviewed changes
Copy link
Member

@huan huan left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Not approved, but need changes.

@huan
Copy link
Member

huan commented Aug 8, 2019

@windmemory Could you please mark my reviews as resolved after you resolve it? Or they will keep showing and it will not very clear for our progress. Thanks.

huan
huan approved these changes Aug 8, 2019
View reviewed changes
src/user/room.ts Outdated Show resolved Hide resolved
src/wechaty.ts Outdated Show resolved Hide resolved
src/wechaty.ts Outdated Show resolved Hide resolved
src/wechaty.ts Outdated Show resolved Hide resolved
huan
huan requested changes Aug 8, 2019
View reviewed changes
Copy link
Member

@huan huan left a comment

Choose a reason for hiding this comment

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

Need changes

@windmemory
Copy link
Member Author

Revised code according to review and one comment to discuss.

huan
huan requested changes Aug 9, 2019
View reviewed changes
Copy link
Member

@huan huan left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for following my reviews!

There only leaves one unnecessary change need to be reverted: please keep the optional arguments optional in on() methods, so that we will not introduce any breaking changes to our end user (high level developers)

After that, we will be able to start making the CI happy.

huan
huan requested changes Aug 9, 2019
View reviewed changes
Copy link
Member

@huan huan left a comment

Choose a reason for hiding this comment

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

When we upgrade our library, the best practice will be kept not to make breaking changes as possible as we can.

If the changes are needed, we should make it compatible and deprecated for one to two major version, before making it a real breaking change.

src/user/room.ts Outdated Show resolved Hide resolved
src/wechaty.ts Outdated Show resolved Hide resolved
@windmemory
Copy link
Member Author

That's right, changed according to review suggestions.

huan
huan previously approved these changes Aug 9, 2019
View reviewed changes
Copy link
Member

@huan huan left a comment

Choose a reason for hiding this comment

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

LGTM.

Please let me know when you want to merge this.

package.json Outdated Show resolved Hide resolved
@huan huan merged commit 4323692 into wechaty:master Aug 9, 2019
@huan
Copy link
Member

huan commented Aug 9, 2019

Merged.

@windmemory windmemory deleted the add-timestamp-to-events branch August 10, 2019 08:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@huan huan huan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@windmemory @huan