-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Bug compatible WXCreateChatRoom #1341
Conversation
Please test your code and confirm it works as you expected. I do not believe it does what you want it to. |
This PR looks better after you commit your new modifications. I'd like to suggest that we should write a new pure function helper to implement this function, maybe And we need to bundle unit tests with the new function, I think there's already two in your comments:
|
Good suggestion! And I suggest:
|
Ok. I'll merge this PR after you finished it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome. Please follow my suggestions before I merge it.
@@ -24,3 +24,4 @@ export * from './room-event-leave-message-parser' | |||
export * from './room-event-topic-message-parser' | |||
export * from './room-raw-payload-parser' | |||
export * from './split-name' | |||
export * from './compatible-wei' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please modify the export files list in order: by filename.
* BUG compitable: "\n\u00135907139882@chatroom" -> "5907139882@chatroom" | ||
* BUG compitable: "\n\u001412558026334@chatroom" -> "12558026334@chatroom" | ||
*/ | ||
export function pureUserName(id?: string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pureUserName -> stripBugWei
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe not stripBugWei
, because I'm sure it will have more bug in the future...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename then we can merge.
@@ -12,6 +12,7 @@ | |||
* | |||
*/ | |||
|
|||
export * from './compatible-wei' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
compatible-wei -> compatible-wei-bug
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This rename is required because that we compatible with wei because it has BUG.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are right, done
see wechaty/wechaty-puppet-padchat#62