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 Pull.head/base, which return (newly-added) PullRef #1125

Merged
merged 9 commits into from
Jun 17, 2015
Merged

Add Pull.head/base, which return (newly-added) PullRef #1125

merged 9 commits into from
Jun 17, 2015

Conversation

cvrebert
Copy link
Contributor

Fixes #1090 by adding Pull.base() and Pull.head() getters, which return instances of the new PullRef class, which allows structured access to the pull request branch+commit data. Thus, the user no longer has to resort to dealing with the raw JSON directly themselves.

@cvrebert
Copy link
Contributor Author

@dmarkov Please, find a reviewer for this.

@cvrebert cvrebert mentioned this pull request Jun 15, 2015
Open
@dmarkov
Copy link

dmarkov commented Jun 15, 2015

@cvrebert I will find a reviewer for your pull requests shortly, thanks for contribution!

@dmarkov
Copy link

dmarkov commented Jun 15, 2015

@pinaf please review, thanks

@cvrebert cvrebert mentioned this pull request Jun 15, 2015
Closed
@cvrebert
Copy link
Contributor Author

@pinaf Friendly ping.

pinaf
pinaf reviewed Jun 16, 2015
View reviewed changes
src/main/java/com/jcabi/github/PullRef.java
/**
* Encapsulated pull request ref.
*/
private final transient PullRef pullref;
Copy link
Contributor

Choose a reason for hiding this comment

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

@cvrebert I'd rename this to ref

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pinaf That would conflict with the ref() method

@pinaf
Copy link
Contributor

pinaf commented Jun 16, 2015

@cvrebert taking a look now

pinaf
pinaf reviewed Jun 16, 2015
View reviewed changes
src/main/java/com/jcabi/github/mock/MkPull.java
).json();
final JsonObjectBuilder json = Json.createObjectBuilder();
for (final Map.Entry<String, JsonValue> val : obj.entrySet()) {
json.add(val.getKey(), val.getValue());
Copy link
Contributor

Choose a reason for hiding this comment

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

@cvrebert you are no longer adding all key/values as before. don't you think this might be an issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pinaf Well, all the unit tests still pass, so no.

@pinaf
Copy link
Contributor

pinaf commented Jun 16, 2015

@cvrebert still a few references to static fields in MkPullTest that need the class name

@pinaf
Copy link
Contributor

pinaf commented Jun 16, 2015

@cvrebert https://github.com/jcabi/jcabi-github/pull/1125/files#diff-d6ad4c257618d2224744964f8437ecbcR223

@pinaf
Copy link
Contributor

pinaf commented Jun 16, 2015

@cvrebert https://github.com/jcabi/jcabi-github/pull/1125/files#diff-d6ad4c257618d2224744964f8437ecbcR224

@pinaf
Copy link
Contributor

pinaf commented Jun 16, 2015

@cvrebert https://github.com/jcabi/jcabi-github/pull/1125/files#diff-d6ad4c257618d2224744964f8437ecbcR212

@pinaf
Copy link
Contributor

pinaf commented Jun 16, 2015

@cvrebert https://github.com/jcabi/jcabi-github/pull/1125/files#diff-d6ad4c257618d2224744964f8437ecbcR180

@pinaf
Copy link
Contributor

pinaf commented Jun 16, 2015

@cvrebert https://github.com/jcabi/jcabi-github/pull/1125/files#diff-d6ad4c257618d2224744964f8437ecbcR176

@pinaf
Copy link
Contributor

pinaf commented Jun 16, 2015

@cvrebert https://github.com/jcabi/jcabi-github/pull/1125/files#diff-d6ad4c257618d2224744964f8437ecbcR156

@pinaf
Copy link
Contributor

pinaf commented Jun 16, 2015

@cvrebert https://github.com/jcabi/jcabi-github/pull/1125/files#diff-d6ad4c257618d2224744964f8437ecbcR145

@cvrebert
Copy link
Contributor Author

@pinaf Made the MkPullTest fixes.

@cvrebert
Copy link
Contributor Author

@pinaf Moved the various method calls up.

@cvrebert
Copy link
Contributor Author

@pinaf Fixed/responded to all your previous comments.

@pinaf
Copy link
Contributor

pinaf commented Jun 17, 2015

@cvrebert thank you!

@pinaf
Copy link
Contributor

pinaf commented Jun 17, 2015

@rultor let's merge

@rultor
Copy link
Contributor

rultor commented Jun 17, 2015

@rultor let's merge

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

@yegor256
Copy link
Member

@rultor merge this

@rultor
Copy link
Contributor

rultor commented Jun 17, 2015

@rultor merge this

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

@rultor rultor merged commit 2486c63 into jcabi:master Jun 17, 2015
@rultor
Copy link
Contributor

rultor commented Jun 17, 2015

@rultor merge this

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

@dmarkov
Copy link

dmarkov commented Jun 17, 2015

@cvrebert once 1125-99a7deed puzzle is resolved (later, in another ticket), this ticket will be fully complete

@cvrebert cvrebert deleted the impl-1090-pr-head-base branch June 17, 2015 09:30
@dmarkov
Copy link

dmarkov commented Jun 18, 2015

@pinaf thank you, added 32 mins to your acc, payment num is AP-7JF92963MW356224X, 47 hours and 27 mins spent in total to complete. extra minutes for review comments (c=17). +32 added to your rating, current score is: +7115

@dmarkov
Copy link

dmarkov commented Jun 18, 2015

@rultor deploy pls

@rultor
Copy link
Contributor

rultor commented Jun 18, 2015

@rultor deploy pls

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

@rultor
Copy link
Contributor

rultor commented Jun 18, 2015

@rultor deploy pls

@dmarkov Done! FYI, the full log is here (took me 7min)

@llorllale llorllale mentioned this pull request Jul 31, 2018
Closed
@0pdd
Copy link

0pdd commented Jul 5, 2022

@cvrebert the puzzle #unknown is still not solved.

@0pdd 0pdd mentioned this pull request Sep 27, 2024
Open
@0pdd
Copy link

0pdd commented Sep 27, 2024

@cvrebert the puzzle #1784 is still not solved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@pinaf pinaf

Labels
@pinaf
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unable to get head or base commits of a pull request
6 participants
@cvrebert @dmarkov @pinaf @rultor @yegor256 @0pdd