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

Added "deleteIssue" function and changed "CurlClient" to support DELETE request #53

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

marek-knappe
Copy link

  • added "deleteIssue" function
  • changed "CurlClient" to support DELETE request

Procta and others added 4 commits August 13, 2014 17:09
Added regular expression to remove trailing slash if included in URL. Api::__construct
Fixed trailing slash in URL
@@ -72,6 +72,9 @@ public function __construct(
AuthenticationInterface $authentication,
ClientInterface $client = null
) {
//Regular expression to remove trailing slash
$endpoint = preg_replace('{/$}', '', $endpoint);
Copy link
Collaborator

Choose a reason for hiding this comment

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

By replace code with solution below no explanatory comment is needed:

$endpoint = rtrim($endpoint, '/');

Sounds like useful change, but not sure how it's related to this PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually just remove it from this PR, because there is another PR doing the same thing: #25 .

* @param $params
* @return mixed
*/
public function getWorklogs($issueKey, $params)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove, because this is already submitted as part of #37.

@aik099
Copy link
Collaborator

aik099 commented Feb 27, 2016

@marek-knappe I see now why you have all your PR code mixed together. You're not using feature branches and submitting PR's from master branch where all is added.

Unfortunately GitHub doesn't allow to change PR branch and therefore the only viable approach is this:

  1. we close the PR
  2. on your repo clone you create a feature branch (e.g. delete-issue-feat) and put new code in there
  3. push it to GitHub
  4. submit new PR from pushed branch

return $this->api(
self::REQUEST_DELETE, sprintf("/rest/api/2/issue/%s", $issueKey),
array (
'deleteSubtasks' => $deleteSubtasks
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please change $deleteSubtasks into $deleteSubtasks ? 'true' : 'false'.

@aik099 aik099 changed the title Added deleteIssue function and changed curlClient to support DELETE r… Added "deleteIssue" function and changed "CurlClient" to support DELETE request Feb 29, 2016
@marek-knappe
Copy link
Author

@aik099
The reason why #53 and #57 are submitted together is because if you would choose to just merge #57 you wouldn't have #53 changes and it wouldn't work :)
anyway, will fix your recommendation and submit another PR

@aik099
Copy link
Collaborator

aik099 commented Feb 29, 2016

The reason why #53 and #57 are submitted together is because if you would choose to just merge #57 you wouldn't have #53 changes and it wouldn't work :)
anyway, will fix your recommendation and submit another PR

In such case I usually submit different PRs and mention somewhere that another PR needs to be merged first for this one to work.

@marek-knappe
Copy link
Author

If you find it better (less going back and forward) I'm happy for you or anyone to make it properly :)

curl_setopt($curl, CURLOPT_POSTFIELDS, json_encode($data));
}
} elseif ($method == "DELETE") {
curl_setopt($curl, CURLOPT_CUSTOMREQUEST, "DELETE");
Copy link
Collaborator

Choose a reason for hiding this comment

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

The DELETE calls can have parameters. The correct implementation is done in #76.

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

Successfully merging this pull request may close these issues.

3 participants