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

Normlize selector use within lotus #7467

Merged
merged 1 commit into from
Oct 7, 2021

Conversation

ribasushi
Copy link
Collaborator

No description provided.

@ribasushi ribasushi requested a review from rvagg October 7, 2021 08:49
@ribasushi ribasushi requested a review from a team as a code owner October 7, 2021 08:49
@codecov
Copy link

codecov bot commented Oct 7, 2021

Codecov Report

Merging #7467 (a40aa9f) into master (1993efe) will decrease coverage by 0.03%.
The diff coverage is 91.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7467      +/-   ##
==========================================
- Coverage   39.21%   39.17%   -0.04%     
==========================================
  Files         631      631              
  Lines       66787    66788       +1     
==========================================
- Hits        26191    26165      -26     
- Misses      36024    36056      +32     
+ Partials     4572     4567       -5     
Impacted Files Coverage Δ
node/impl/client/client.go 45.11% <91.66%> (+0.07%) ⬆️
journal/types.go 86.66% <0.00%> (-13.34%) ⬇️
chain/actors/builtin/miner/diff.go 48.52% <0.00%> (-10.30%) ⬇️
chain/events/observer.go 71.64% <0.00%> (-6.72%) ⬇️
extern/sector-storage/manager_calltracker.go 57.70% <0.00%> (-4.85%) ⬇️
chain/actors/builtin/paych/paych.go 14.77% <0.00%> (-4.55%) ⬇️
chain/exchange/peer_tracker.go 66.66% <0.00%> (-4.31%) ⬇️
node/hello/hello.go 63.63% <0.00%> (-3.41%) ⬇️
journal/fsjournal/fs.go 66.66% <0.00%> (-2.78%) ⬇️
itests/kit/blockminer.go 93.65% <0.00%> (-1.59%) ⬇️
... and 15 more

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 1993efe...a40aa9f. Read the comment docs.

@ribasushi ribasushi force-pushed the chore/normalize-recursive-sel-references branch from ff1ed89 to a40aa9f Compare October 7, 2021 09:01
Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

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

I've been slightly hesitant to do because it's not precisely the same selector, but functionally should be.

We could simplify further by cleaning up the textselector API I reckon, but that's not critical and I think we should work on integrating that functionality into go-ipld-prime and clarifying the API when doing that.

@ribasushi
Copy link
Collaborator Author

@rvagg
Copy link
Member

rvagg commented Oct 7, 2021

I think it's just a simple matter of key ordering, I haven't spent more than a couple of brain cycles on parsing this, but ssb.ExploreRecursive(selector.RecursionLimitNone(), ssb.ExploreAll(ssb.ExploreRecursiveEdge())).Node() is:

{"R":{":>":{"a":{">":{"@":{}}}},"l":{"none":{}}}}

while CommonSelector_ExploreAllRecursively is:

{"R":{"l":{"none":{}},":>":{"a":{">":{"@":{}}}}}}

The difference alone was enough to make be baulk before diving in, but I think it's functionally the same, just comes out different when dag-jsonified.

@magik6k magik6k merged commit 734a03c into master Oct 7, 2021
@magik6k magik6k deleted the chore/normalize-recursive-sel-references branch October 7, 2021 18:05
dirkmc pushed a commit to filecoin-project/go-fil-markets that referenced this pull request Oct 11, 2021
* Standardize definition of a recursive non-matching selector

In the spirit of filecoin-project/lotus#7467

* Proprly deprecate as understood by the golang toolchain

Co-authored-by: Daniel Martí <mvdan@mvdan.cc>

Co-authored-by: Daniel Martí <mvdan@mvdan.cc>
rvagg pushed a commit to filecoin-project/go-fil-markets that referenced this pull request Oct 15, 2021
* Standardize definition of a recursive non-matching selector

In the spirit of filecoin-project/lotus#7467

* Proprly deprecate as understood by the golang toolchain

Co-authored-by: Daniel Martí <mvdan@mvdan.cc>

Co-authored-by: Daniel Martí <mvdan@mvdan.cc>
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.

3 participants