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

Implement queryRecord (introduced by Ember 1.13) #36

Merged
merged 1 commit into from
Sep 2, 2015

Conversation

sebweaver
Copy link
Collaborator

Hello (again),

Not really a compatibility issue, but I thought suggesting an implementation for the new queryRecord could help. For now, this implementation returns the first record matching the given query (instead of an array) or reject if not found (instead of an empty array).

Happy to discuss that further if you have another idea of implementation.

I added the related tests.

Regards,

Sébastien

@frederikbosch
Copy link
Contributor

@sebweaver Let me read upon this. Will come back to it.

BTW: Same counts for pushPayload at the serializer level. I just did some tests with the REST pushPayload method, and I think there are just a few tricks needed to get it working.

frederikbosch added a commit that referenced this pull request Sep 2, 2015
Implement `queryRecord` (introduced by Ember 1.13)
@frederikbosch frederikbosch merged commit 44e9ffb into genkgo:master Sep 2, 2015
@frederikbosch
Copy link
Contributor

Thanks again @sebweaver!! I fixed one issue in #37. In your PR, the result of the last test was used instead of asserting that all tests should pass. I also made some readibility improvements. Nonetheless, thanks for contributing: your efforts are much appreciated! This will be tagged tomorrow.

@sebweaver
Copy link
Collaborator Author

You're welcome.

Good news for the issue you've fixed. It didn't jump out at me when I added the new argument. Perhaps we should add a test on that case, because the existing tests don't cover it.

Actually and more generally, my intention and my effort were mainly focused on making the package compatible with 1.13, not on optimizing, restyling or revamping the working (or not) pieces of code. Mixing too many objectives per PR could lead to introduce new issues much more difficult to track: will the issues be related to the migration or due to the rewriting? One objective at a time also keep the diff more concise and readable.

For example, here is a few ideas I had, during the upgrade process, for further dedicated PRs:

  • Clean the code following the JS style guides (like this one and/or this one)
  • Migrate to ES6 syntax
  • Complete missing features to cover the whole API of DS.Store (i.e. pushPayload)
  • Override the new JSONAPISerializer rather than the JSONSerializer (to be discussed)
  • Stick on every ED release

It would be my pleasure to take part in them!

I also have an idea about a new implementation to discuss with you, but shush... it's too soon, I'm too new on the projet... ;-)

@frederikbosch
Copy link
Contributor

@sebweaver Made start on the clean code, by reformatting whitespaces, and with the ES6 syntax by start using the arrow syntax. On the pushPayload: there has reported a bug already for this issue, see #38. Is there more missing? Then we could start by making issues for those.

Sticking on every release is fine with me. See my comment on how to deal with versions in the current readme.

@frederikbosch
Copy link
Contributor

@sebweaver And just added a test for the query bugfix, together with a new release 1.13.1.

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