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

#239 PsLinkedin passes the request parameters to the URI. #245

Merged
merged 5 commits into from
May 11, 2015
Merged

#239 PsLinkedin passes the request parameters to the URI. #245

merged 5 commits into from
May 11, 2015

Conversation

super132
Copy link
Contributor

@super132 super132 commented May 2, 2015

For task #239, removing request parameters in Href object and move them into body.

@super132
Copy link
Contributor Author

super132 commented May 2, 2015

@davvd , this is fix for #239, please review. Thanks.

@davvd
Copy link

davvd commented May 2, 2015

@super132 Thanks, let me find someone who can review this pull request

@davvd
Copy link

davvd commented May 2, 2015

@caarlos0 please review, thanks

@davvd davvd added CR labels May 2, 2015
@caarlos0
Copy link

caarlos0 commented May 3, 2015

@super132 can you write some test?

@super132
Copy link
Contributor Author

super132 commented May 3, 2015

@caarlos0, I can write some tests but now there is no test for PsLinkedin and there is puzzle for it. Does it mean that I need to solve it together with that puzzle? Thanks.

@caarlos0
Copy link

caarlos0 commented May 3, 2015

@super132 oh, if there is a puzzle already, I think we are good to go here..

@caarlos0
Copy link

caarlos0 commented May 3, 2015

@rultor good to merge

@rultor
Copy link
Collaborator

rultor commented May 3, 2015

@rultor good to merge

@caarlos0 Thanks for your request. @yegor256 Please confirm this.

@yegor256
Copy link
Owner

yegor256 commented May 4, 2015

@super132 @caarlos0 no, we're not good to go. we should wait for that puzzle resolution and then proceed here with a unit test

@super132
Copy link
Contributor Author

super132 commented May 4, 2015

@yegor256 @caarlos0 understood. Let's wait for #158 to be resolved in order to add a test. Thanks.

@caarlos0
Copy link

caarlos0 commented May 4, 2015

@yegor256 @super132 makes sense.

@caarlos0
Copy link

caarlos0 commented May 4, 2015

@davvd we are waiting for #158 here.

@davvd
Copy link

davvd commented May 5, 2015

@davvd we are waiting for #158 here.

@caarlos0 sure, let's wait for #158

@davvd
Copy link

davvd commented May 10, 2015

@super132 #158 is closed, it was an impediment

@super132
Copy link
Contributor Author

@caarlos0 I have amended the login test case to check for request body and URL. Please have a look. Thanks.

@caarlos0
Copy link

@super132 looks good.

@caarlos0
Copy link

@rultor good to merge

@rultor
Copy link
Collaborator

rultor commented May 10, 2015

@rultor good to merge

@caarlos0 Thanks for your request. @yegor256 Please confirm this.

new Take() {
@Override
public Response act(final Request req) throws IOException {
final BufferedInputStream buffer =
Copy link
Owner

Choose a reason for hiding this comment

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

@super132 I would rather use RqPrint(req).printBody()

@yegor256
Copy link
Owner

@super132 there is a better/shorter way to do what you're doing, see above

@super132
Copy link
Contributor Author

@yegor256 thanks for you advice. I have amended it using RqPrint, please take a look. Thanks.

.append(lapp)
.toString(),
"redirect_uri=",
new StringBuffer("client_secret=")
Copy link
Owner

Choose a reason for hiding this comment

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

@super132 don't you think that String.format() is more suitable here? see http://www.yegor256.com/2014/06/19/avoid-string-concatenation.html

@yegor256
Copy link
Owner

@super132 one more comment, see above

@super132
Copy link
Contributor Author

@yegor256 right, it would be cleaner to use String.format() instead. I have revised the code. Thanks.

@yegor256
Copy link
Owner

@rultor try to merge

@rultor
Copy link
Collaborator

rultor commented May 11, 2015

@rultor try to merge

@yegor256 OK, I'll try to merge now. You can check the progress of the merge here

@rultor rultor merged commit f840583 into yegor256:master May 11, 2015
@rultor
Copy link
Collaborator

rultor commented May 11, 2015

@rultor try to merge

@yegor256 Done! FYI, the full log is here (took me 12min)

@davvd
Copy link

davvd commented May 12, 2015

@elenavolokhova please, let us know what do you think about this ticket, according to our QA rules

@davvd
Copy link

davvd commented May 12, 2015

@rultor deploy now pls

@rultor
Copy link
Collaborator

rultor commented May 12, 2015

@rultor deploy now pls

@davvd OK, I'll try to deploy now. You can check the progress here

@rultor
Copy link
Collaborator

rultor commented May 12, 2015

@rultor deploy now pls

@davvd Done! FYI, the full log is here (took me 10min)

@elenavolokhova
Copy link

@caarlos0
According to our quality rules:

The code reviewer found at least three problems in the code.

Please confirm that you will try to find at least three problems next time.

@caarlos0
Copy link

@elenavolokhova Yes, I'll try

@elenavolokhova
Copy link

@davvd Quality is acceptable here.

@davvd
Copy link

davvd commented May 13, 2015

@davvd Quality is acceptable here.

@elenavolokhova thanks, got it. everybody, please try to make it good next time

@davvd
Copy link

davvd commented May 13, 2015

@caarlos0 just added 10 mins to @elenavolokhova (for QA), payment ID is 57255929; paid, thanks, added 23 mins to your account, payment AP-1JP12351VU9246113, 21 hours and 55 mins was spent; extra minutes for review comments (c=8); added +23 to your rating, now it is equal to +1952

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants