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

Fixed return value of patch method for postgresql dialect #104

Merged
merged 1 commit into from
Apr 28, 2017

Conversation

msimulcik
Copy link
Contributor

Fixed return value of patch method for postgresql dialect

  • Tell us about the problem your pull request is solving.
    For the postgresql dialect, the patch method is returning an array even if an ID is specified. This pull request makes the patch behaviour consistent.
  • Are there any open issues that are related to this?
    No
  • Is this PR dependent on PRs in other repos?
    No

@daffl
Copy link
Member

daffl commented Apr 27, 2017

This seems to be passing but I'm not sure I understand why unless the tests are missing something.

@msimulcik
Copy link
Contributor Author

Do tests cover dialect differences, because I can't find it? I can add failing test, but pls point me to a repo where it belongs. Is it feathers-service-tests?

@daffl
Copy link
Member

daffl commented Apr 27, 2017

Oh sorry, I see this was in a dialect specific branch. What I mean is that this looks like it would be a breaking change for anybody who is using the Postgres dialect because you used to get results[1] back and now get results[1][0] right?

@msimulcik
Copy link
Contributor Author

I think breaking change was introduced by #93 because original behaviour of patch was to return single result when id was specified. I noticed this when I updated feathers-sequelize because I'm using postgresql dialect.

@daffl daffl merged commit ca107fb into feathersjs-ecosystem:master Apr 28, 2017
@daffl
Copy link
Member

daffl commented Apr 28, 2017

This does look like the right fix, let's hope this works (@MichaelErmer sorry if we're breaking the build).

@msimulcik
Copy link
Contributor Author

Thanks for merging it, but I still think we should add a test covering this dialect branch. I'm happy to do it, I just need to know where it belongs.

@daffl
Copy link
Member

daffl commented Apr 28, 2017

Good call, I haven't released it yet. We have to set up PostgreSQL on Travis CI and then run the base tests against that database, probably by initializing a new sequelize and app with that connection.

@daffl
Copy link
Member

daffl commented Apr 28, 2017

I created follow-up issue #105

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