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

Change withURLPath to setRawURLPath #770

Merged
merged 9 commits into from
Apr 27, 2020

Conversation

sladyn98
Copy link
Contributor

@sladyn98 sladyn98 commented Apr 2, 2020

Description

This PR deals with trying to fiinish moving all full URL setting to setRawUrlPath and change withUrlPath to not require starting /
Related to #700

Before submitting a PR:

We love getting PRs, but we hate asking people for the same basic changes every time.

  • Push your changes to a branch other than master. Create your PR from that branch.
  • Write tests that run and pass in CI. See CONTRIBUTING.md for details on how to capture snapshot data.
  • Run mvn clean compile locally. This may reformat your code, commit those changes.
  • Run mvn -D enable-ci clean install site locally. If this command doesn't succeed, your change will not pass CI.

@sladyn98
Copy link
Contributor Author

sladyn98 commented Apr 3, 2020

I have started to change some methods, would the PR involve changing all of the methods to setRawURLPath
CC @bitwiseman

@bitwiseman
Copy link
Member

@sladyn98 Thanks but this isn't what I'm looking for with #700. setRawUrlPath should only be used on when the url being passed in is a full url like the ones returned get from GHObject.getUrl(). All the tailUrl strings should remain withUrlPath.

@sladyn98 sladyn98 force-pushed the change_url_methods branch from b2c9b2a to 6a176e6 Compare April 6, 2020 12:44
@@ -118,7 +118,7 @@ public String getSHA1() {
* if disabling protection fails
*/
public void disableProtection() throws IOException {
root.createRequest().method("DELETE").withUrlPath(protection_url).send();
root.createRequest().method("DELETE").setRawUrlPath(protection_url).send();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bitwiseman protection_url is a full url http://localhost:36413/repos/github-api-test-org/temp-testDisableProtectionOnly/branches/master/protection, so I changed it , is this similar to what you have in mind

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will add the wire mock snapshot soon.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this is the kind of change that I'm looking for.

@sladyn98 sladyn98 force-pushed the change_url_methods branch from d9ed3bb to f48b33e Compare April 7, 2020 12:47
@bitwiseman
Copy link
Member

@sladyn98
Before you continue with this PR: please rebase your branch from https://github.com/github-api/github-api master and then re-record your test files. #756 was supposed to result in shorter resource file names and yours are still long.

@sladyn98
Copy link
Contributor Author

@bitwiseman Definitely I will rebase and re re record

@sladyn98 sladyn98 force-pushed the change_url_methods branch 2 times, most recently from f48b33e to 5837cf4 Compare April 20, 2020 08:01
Copy link
Contributor Author

@sladyn98 sladyn98 left a comment

Choose a reason for hiding this comment

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

@bitwiseman A little help required

public void testGetProtection() throws Exception {
GHBranchProtection protection = branch.enableProtection().enable();
GHBranchProtection protectionTest = repo.getBranch(BRANCH).getProtection();
assertTrue(protectionTest instanceof GHBranchProtection);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even though the above branch master is already protected it still throws a message that the branch is not protected. It seems a bit strange

 "body": "{\"message\":\"Branch not protected\",\"documentation_url\":\"https://developer.github.com/v3/repos/branches/#remove-branch-protection\"}",

Copy link
Member

Choose a reason for hiding this comment

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

GitHub doesn't think it is protected. The data files definitely show that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there any solution ? Because manually changing doesn't seem to work

Copy link
Member

Choose a reason for hiding this comment

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

@sladyn98
Not sure. I'll take a look.

@sladyn98
Copy link
Contributor Author

sladyn98 commented Apr 24, 2020

@bitwiseman Wonderful the tests now pass :)
Should we break down the changes class by class so I did not find any more usages in this class so I guess this PR is ready to be merged.

@sladyn98 sladyn98 requested a review from bitwiseman April 24, 2020 09:28
@bitwiseman
Copy link
Member

@sladyn98 Class (or even smaller) is fine. Sounds good.

@sladyn98
Copy link
Contributor Author

@bitwiseman this PR added some tests, so it's kind of an improvement :)

@bitwiseman
Copy link
Member

Thanks @sladyn98 !

@bitwiseman bitwiseman merged commit 2c8c678 into hub4j:master Apr 27, 2020
@sladyn98
Copy link
Contributor Author

@bitwiseman Your welcome looking forward to contributing more :)

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.

2 participants