-
Notifications
You must be signed in to change notification settings - Fork 202
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
11: Pass for Twitter added. #175
Conversation
@popprem Thanks, I will find someone to review this PR soon |
@longtimeago please review,thanks |
@@ -136,6 +136,11 @@ | |||
<optional>true</optional> | |||
</dependency> | |||
<dependency> | |||
<groupId>commons-codec</groupId> |
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.
@popprem we are not allowed to use external dependency with compile scope. We promised that to our clients.
javax.xml.bind.DatatypeConverter
should help
@popprem thanks for the PR. See several remarks above |
@longtimeago Thanks for the review, have addressed the issues. Please check. |
@popprem Now we are good to go, thank you! |
@rultor merge please |
@longtimeago Thanks for your request. @yegor256 Please confirm this. |
@popprem looks like I've missed to note one important thing ... no tests ?? Please, add at least one. Yegor will not leave out this PR without test |
@longtimeago Yes, I have commented the same on the pull request. Test for PsTwitter is not straightforward ,should be done with oauth mocks etc. Need advise on the same. |
@popprem yep ... I see the comment. Add at least todo in the code. Other developer will implement it |
@longtimeago Test added with todo comment, please check. |
@popprem Todo must be formatted as puzzle (see examples https://github.com/teamed/pdd) |
@longtimeago Test description updated with proper todo format. Please verify. |
@popprem I still don't understand why do you use 8 indentation spaces sometimes and sometimes 4. what is the message here? what are you trying to say by 8 spaces? also, read this: http://www.yegor256.com/2014/10/23/paired-brackets-notation.html |
@yegor256 Looks cleaner now, please verify. |
@rultor merge |
@yegor256 Oops, I failed. You can see the full log here (spent 1min)
|
@popprem please, merge from master |
@longtimeago Have merged from master. Please try merging. |
@rultor please merge |
@longtimeago Thanks for your request. @yegor256 Please confirm this. |
@yegor256 Can this be merged please? |
@rultor merge |
@popprem it is closed already |
@elenavolokhova review this ticket please, for compliance with our quality rules |
@rultor deploy pls |
@longtimeago |
@elenavolokhova Added link to the original ticket. |
@popprem Thank you! |
@davvd Quality is acceptable |
@elenavolokhova thanks for the review, we'll try better next time |
@elenavolokhova thanks a lot, next time everybody should try to make it better |
@longtimeago I added 10 mins to @elenavolokhova (for QA review) in transaction 56850215... Thanks a lot, I just topped your account for 26 mins, transaction ID |
Pass for twitter added. Todo: PsTwitterTest has to be implemented or XeTwitterLink & XeTwitterLinkTest have to be implemented which will test PsTwitter too.
PR #175 for issue #11.