Skip to content

Conversation

@AnthonySendra
Copy link
Contributor

No description provided.

@codecov-io
Copy link

codecov-io commented Mar 30, 2017

Codecov Report

Merging #200 into 4.x will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##              4.x     #200      +/-   ##
==========================================
+ Coverage   99.35%   99.36%   +<.01%     
==========================================
  Files          16       16              
  Lines        1718     1720       +2     
  Branches      458      459       +1     
==========================================
+ Hits         1707     1709       +2     
  Misses         11       11
Impacted Files Coverage Δ
src/Document.js 100% <100%> (ø) ⬆️
src/Room.js 100% <100%> (ø) ⬆️
src/Collection.js 96.96% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b1d2eb0...d30617a. Read the comment docs.

Copy link
Contributor

@scottinet scottinet left a comment

Choose a reason for hiding this comment

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

The code looks good to me, but I'm not fond of the kuzzleInfo name. I mean, without context, my first thought is that this property provides informations about Kuzzle.

This also opens a (hopefully quick) discussion on how to name this property in our API (maybe not for the internal storage name, as this name pretty much ensures that there won't be collisions with actual documents).
When documents are exported by our API (and exposed in our SDKs), I think it might be better to rename it for something clearer, like _properties for instance.

@AnthonySendra
Copy link
Contributor Author

@scottinet totally agree with you. I thought it was a good idea to keep the same name between attribute returned by the API and the attribute returned by the SDK.
Do we have to open an issue on kuzzle side and let this PR in WIP?

@AnthonySendra AnthonySendra dismissed scottinet’s stale review April 12, 2017 09:02

The kuzzleInfo attribute is now called meta

@AnthonySendra AnthonySendra self-assigned this Apr 14, 2017
@jenow jenow merged commit 02f71e8 into 4.x Apr 14, 2017
@jenow jenow deleted the 18-kuzzle-info branch April 14, 2017 12:37
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.

6 participants