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

Add getValueByPath() to the document #86

Merged
merged 6 commits into from
Jul 13, 2023
Merged

Add getValueByPath() to the document #86

merged 6 commits into from
Jul 13, 2023

Conversation

humdrum
Copy link
Contributor

@humdrum humdrum commented Jul 11, 2023

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:


Additional documentation:


Checklist:

  • Added relevant tests or not required
  • Didn't break anything

@humdrum humdrum self-assigned this Jul 11, 2023
@codecov
Copy link

codecov bot commented Jul 12, 2023

Codecov Report

Merging #86 (5b15cb8) into main (840e723) will increase coverage by 0.15%.
The diff coverage is 85.71%.

@@            Coverage Diff             @@
##             main      #86      +/-   ##
==========================================
+ Coverage   55.90%   56.05%   +0.15%     
==========================================
  Files          92       92              
  Lines       14637    14695      +58     
==========================================
+ Hits         8183     8238      +55     
- Misses       6454     6457       +3     
Impacted Files Coverage Δ
Sources/Document/Change/Change.swift 100.00% <ø> (ø)
Sources/Document/Json/JSONObject.swift 86.40% <50.00%> (-0.66%) ⬇️
Sources/Util/LLRBTree.swift 85.99% <60.00%> (ø)
Sources/Document/Document.swift 50.36% <82.14%> (+3.59%) ⬆️
Tests/Unit/Document/DocumentTests.swift 98.59% <100.00%> (+0.03%) ⬆️
Tests/Unit/Document/JSONObjectTests.swift 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

Task {
let array = try? await target.getValueByPath(path: "$.") as? JSONObject

XCTAssertTrue(array != nil)

Choose a reason for hiding this comment

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

Can we verify if the value is properly returned instead of checking if the array is not nil?
The getValueByPath() method is used to retrieve values, such as doc.getValueByPath('$.obj.num'), instead of using doc.getRoot().obj.num. Therefore, it would be better to verify if the value is properly returned.

(Refer to the example using getValueByPath in the js-sdk:
https://github.com/yorkie-team/yorkie-js-sdk/blob/395d052f2a6a3c1a5cef794a2e45b0312a63e945/public/multi.html#L169 )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added some Test code that verifies the value of getValueByPath() results.

Choose a reason for hiding this comment

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

Thank you for the modification!
I have a question: Is it better to silently ignore the usage of a key containing . instead of explicitly throwing an error?

root["$$...hello"] = [] as [Any]

// The key name can't contain "."
XCTAssertTrue(root["$$...hello"] == nil)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will add an assert and modify it so that you can know explicitly whether the wrong value was used in the development.

Copy link
Contributor

@myupage myupage left a comment

Choose a reason for hiding this comment

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

Looks good to me :)

@hackerwins hackerwins changed the title add getValueByPath() to the document Add getValueByPath() to the document Jul 13, 2023
@hackerwins hackerwins merged commit fb0bf88 into main Jul 13, 2023
2 checks passed
@hackerwins hackerwins deleted the getValueByPath branch July 13, 2023 10:56
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.

4 participants