Skip to content

Support for Http patch method in RestHelper - #1874

Merged
farmdawgnation merged 3 commits into
lift:masterfrom
ricsirigu:http-patch-method
Jul 11, 2017
Merged

Support for Http patch method in RestHelper#1874
farmdawgnation merged 3 commits into
lift:masterfrom
ricsirigu:http-patch-method

Conversation

@ricsirigu

Copy link
Copy Markdown
Contributor

@farmdawgnation farmdawgnation left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you please add a test to RestHelperSpec that tests this? There should already be a test in there for OPTIONS so it should be pretty straightforward to add one for PATCH.

@farmdawgnation farmdawgnation added this to the 3.2.0-M1 milestone Jul 3, 2017
@ricsirigu

Copy link
Copy Markdown
Contributor Author

@farmdawgnation more tests coming soon

@farmdawgnation farmdawgnation left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This LGTM.

If you have more tests you want to add that's fine, but I think this is sufficient for now. We need to improve testing writ large, but this brings the testing up to the level that the rest of it is currently at.

Barring any concerns being surfaced in the next day or so, I'll probably go ahead and merge this.

@ricsirigu

Copy link
Copy Markdown
Contributor Author

I would like to write more test but I haven't found the time yet.
If you agree I can send another pull request when more tests will be ready.

@farmdawgnation

Copy link
Copy Markdown
Member

If you agree I can send another pull request when more tests will be ready.

Of course.

@farmdawgnation
farmdawgnation merged commit b4d23d4 into lift:master Jul 11, 2017
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