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

Hotfix/remove patch #206

Merged
merged 6 commits into from
Mar 29, 2017
Merged

Hotfix/remove patch #206

merged 6 commits into from
Mar 29, 2017

Conversation

waldemarnt
Copy link

@waldemarnt waldemarnt commented Mar 28, 2017

We are removing the PATCH support due the issue #203 .

  •  General Comment
  • Coverage Status
    Coverage decreased (-1.5%) to 82.699% when pulling b4d257e on hotfix/remove-patch into 6bd13df on develop.
  • Coverage Status
    Coverage decreased (-1.5%) to 82.699% when pulling b4d257e on hotfix/remove-patch into 6bd13df on develop.
  • LGTM
  •  File: test/includes.spec.js:L13-49
  • Could you explain this change? It seems you followed an TODO comment that led you to this change.
  • I had to rewrite tests on which PATCH were used to use PUT instead
  •  File: test/includes.spec.js:L13-49
  • Didn't change the test behaviour, it's passing, I just changed it to be an object as the PUT method expects.

  


This change is Reviewable







@coveralls
Copy link

Coverage Status

Coverage decreased (-1.5%) to 82.699% when pulling b4d257e on hotfix/remove-patch into 6bd13df on develop.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.5%) to 82.699% when pulling b4d257e on hotfix/remove-patch into 6bd13df on develop.

@@ -13,30 +13,36 @@ describe("includes", function () {
function setupLinks(_ids) {
ids = _ids;

function link(url, path, value) {
function link(resource, resourceId, link, linkId) {
const url = `/${resource}/${resourceId}`;
Copy link

@lucaslago lucaslago Mar 28, 2017

Choose a reason for hiding this comment

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

Could you explain this change? It seems you followed a TODO comment that led you to this change.

Copy link
Author

Choose a reason for hiding this comment

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

I had to rewrite tests on which PATCH were used to use PUT instead

op: 'replace',
path: path,
value: value
var data = {

Choose a reason for hiding this comment

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

data is an object now, is this intentional? What's the reason behind this contract change?

Copy link
Author

Choose a reason for hiding this comment

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

Didn't change the test behaviour, it's passing, I just changed it to be an object as the PUT method expects.

@ssebro
Copy link

ssebro commented Mar 28, 2017

LGTM

@waldemarnt waldemarnt merged commit d30ecc9 into develop Mar 29, 2017
@waldemarnt waldemarnt mentioned this pull request Mar 29, 2017
@ShurakaiSoft ShurakaiSoft deleted the hotfix/remove-patch branch September 28, 2017 20:34
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.

None yet

5 participants