Get links Count #1568

StaticallyTypedAnxiety · 2019-07-02T20:12:38Z

PR summary

Allow the zome developer to be able to get the link count

testing/benchmarking notes

( if any manual testing or benchmarking was/should be done, add notes and/or screenshots here )

followups

( any new tickets/concerns that were discovered or created during this work but aren't in scope for review here )

changelog

Please check one of the following, relating to the CHANGELOG-UNRELEASED.md

this is a code change that effects some consumer (e.g. zome developers) of holochain core so it is added to the CHANGELOG-UNRELEASED.md (linked above), with the format - summary of change [PR#1234](https://github.com/holochain/holochain-rust/pull/1234)
this is not a code change, or doesn't effect anyone outside holochain core development

…lochain-rust into get-links-count

StaticallyTypedAnxiety · 2019-07-02T20:15:51Z

I propose a follow up story in which GetLinks uses the NetworkQuery for the querying of the GetLinks.

Could have been done in here but seems out of scope

StaticallyTypedAnxiety · 2019-07-02T20:25:54Z

Still missing get_by_tag functions but that should be coming soon

…lochain-rust into get-links-count

lucksus

Looks good but I really think we should include a refactoring that merges all these new actions, reducers and futures into just one set of those all queries..

lucksus · 2019-07-12T08:08:53Z

core/src/action.rs

@@ -160,8 +160,11 @@ pub enum Action {
    /// Last string is the stringified process unique id of this `hdk::get_links` call.
    GetLinks(GetLinksKey),
    GetLinksTimeout(GetLinksKey),
+    GetLinksCount((GetLinksKey, Option<CrudStatus>)),


We discussed merging Get* / RespondGet* to Query / RespondQuery. That would make this PR already smaller as we would need all that cruft including a new future etc.

I thought about this and just used GetLinks with an enum because one thing I was worried about was the disconnect between what happens on the from the request point of view and the sever point of view. The reason why GetLinksCount was implemented explicitly because following it down the workflow is easy in terms of "new developers" and someone who isn't just new to the system. But I just used an enum as a parameter instead because doesn't over complicate what is happening there.

RespondGet merges and RespondQuery I think is something that should come into play as a later PR imo because some of this stuff can really propagate through a lot of changes making the scope much bigger as it goes.

zippy

This all looks great, especially the tests which cover the different cases. But, as per Nico's suggestion, and my rule that when we repeat things (i.e. copy over boilerplate) for the third time then it's time to generalize. So my requested change is to refactor at least the GetLink* and RespondGetLink* actions and futures into just one Query/RespondQuery with an enum as a the payload that holds either the actual Links result or the count.

So much of the code is the same, that it really seems like a refactor is in order here, and also worth it, because I'm guessing that we may want to be able to add some other enum into list.

CHANGELOG-UNRELEASED.md

zippy · 2019-07-05T18:48:51Z

app_spec/test/test.js

@@ -851,6 +851,137 @@ scenario('get_links_crud', async (s, t, { alice, bob }) => {
  t.equal("deleted",bob_posts_all.Ok.links[1].status);


+})
+
+scenario('get_links_crud_count', async (s, t, { alice, bob }) => {


also need to add this test into app_spec_proc

I thought app_spec_proc copies over this from test.js?

app_spec/test/test.js

lucksus

This looks way better! I still think we should go all the way only talk about queries in our network actions/reducers as mentioned in my inline comments. Also representing GetLinksCount as its own query seems much cleaner to me.

But the main thing here: the new app-spec test is not passing.

lucksus · 2019-07-17T08:04:14Z

core/src/network/query.rs

 #[derive(Debug, Serialize, Deserialize, PartialEq, DefaultJson, Clone)]
 pub enum NetworkQuery {
    GetEntry,
-    GetLinks(String, String),
+    GetLinks(String, String, Option<CrudStatus>, GetLinksNetworkQuery),


I regard link counts as a different query, instead of a parameter to GetLinks. I think it would be much cleaner to represent it here in that way too: instead of introducing those enums above, why don't we have a GetLinksCount(String, String) specialization here?

lucksus · 2019-07-17T08:05:19Z

app_spec/test/test.js

+
+  //make sure  it is equal to 1
+  t.equal(1,alice_posts_deleted.Ok.count);
+  t.equal(1,bob_posts_deleted.Ok.count);


These assertions actually fail: https://circleci.com/gh/holochain/holochain-rust/31280?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

lucksus · 2019-07-17T08:10:26Z

core/src/action.rs

-    RespondGetLinks((QueryEntryData, Vec<(Address, CrudStatus)>, String, String)),
-    HandleGetLinksResult((Vec<(Address, CrudStatus)>, GetLinksKey)),
+    RespondGetLinks((QueryEntryData, GetLinksNetworkResult, String, String)),
+    HandleGetLinksResult((GetLinksNetworkResult, GetLinksKey)),


I would love to see these actions and the corresponding action creators and reducers in the network module to not know about links at all and just operate on the generic level of queries.

…lochain-rust into get-links-count

@zippy

Checked in with @zippy via chat who is on the go rigth now.

…lochain-rust into get-links-count

StaticallyTypedAnxiety and others added 11 commits July 2, 2019 10:47

implemented get links system

a1e1eb5

Handle get links result implemented

35be08a

put handle get links count

36003f0

Implemented nucleaus

2f1e016

Implementing filter on get_links

bac4799

Added query using crud

b16a4fc

Fixed up some misisng code

4ac5a43

cargo fmt

29bcc94

Merge branch 'develop' into get-links-count

aa9bfaf

update proc macro

550e21e

Merge branch 'get-links-count' of https://www.github.com/holochain/ho…

8dcaaa3

…lochain-rust into get-links-count

StaticallyTypedAnxiety changed the title ~~Get links Count~~ [WIP]Get links Count Jul 2, 2019

StaticallyTypedAnxiety added the WIP work in progress label Jul 2, 2019

StaticallyTypedAnxiety and others added 15 commits July 2, 2019 16:57

Update lib.rs

2687c7a

GetLinksCount enum

656afcb

Made some changes for the tag

e24601e

get_links_by_tag implemented

0f77919

Update lib.rs

85c5d4f

Merge branch 'develop' into get-links-count

5380335

Fixed problem with loop

504364a

Updates to timeout

cbd0ec3

added get by tag test

e34657c

Merge branch 'get-links-count' of https://www.github.com/holochain/ho…

b8765df

…lochain-rust into get-links-count

updated spacing

a24cf72

Update lib.rs

408107c

Update lib.rs

89ebdeb

Update CHANGELOG-UNRELEASED.md

169cfdd

updated app_spec

6a2f87c

Merge branch 'develop' into get-links-count

6438ee8

lucksus reviewed Jul 12, 2019

View reviewed changes

zippy previously requested changes Jul 12, 2019

View reviewed changes

StaticallyTypedAnxiety added 4 commits July 15, 2019 11:52

Changed to use enums

eca611a

links refactor

1a46d86

cargo fmt

0f2d83b

added case in test where different tag is added

168f4e2

StaticallyTypedAnxiety requested review from lucksus and zippy July 15, 2019 18:15

StaticallyTypedAnxiety added 5 commits July 15, 2019 18:06

Merge branch 'develop' into get-links-count

18e53f7

Update lib.rs

59554e3

tigger cI

1617097

Update test.js

bc566d4

Merge branch 'develop' into get-links-count

50a8c4a

lucksus reviewed Jul 17, 2019

View reviewed changes

StaticallyTypedAnxiety added 3 commits July 17, 2019 08:43

changes to zome

247338c

Merge branch 'get-links-count' of https://www.github.com/holochain/ho…

83c339b

…lochain-rust into get-links-count

changes to zome

6d0f1fc

lucksus approved these changes Jul 17, 2019

View reviewed changes

Merge branch 'develop' into get-links-count

f8454ec

StaticallyTypedAnxiety and others added 7 commits July 17, 2019 12:37

fixed app_spec

df3700c

Merge branch 'get-links-count' of https://www.github.com/holochain/ho…

193441a

…lochain-rust into get-links-count

Merge branch 'develop' into get-links-count

97d67fa

trigger CI

04b6285

Merge branch 'get-links-count' of https://www.github.com/holochain/ho…

9982399

…lochain-rust into get-links-count

trigger CI

d369c77

trigger CI again

9645f52

StaticallyTypedAnxiety merged commit 3930874 into develop Jul 17, 2019

zippy deleted the get-links-count branch October 4, 2019 18:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get links Count #1568

Get links Count #1568

StaticallyTypedAnxiety commented Jul 2, 2019

StaticallyTypedAnxiety commented Jul 2, 2019 •

edited

Loading

StaticallyTypedAnxiety commented Jul 2, 2019

lucksus left a comment

lucksus Jul 12, 2019

StaticallyTypedAnxiety Jul 15, 2019

zippy left a comment

zippy Jul 5, 2019

StaticallyTypedAnxiety Jul 15, 2019

lucksus left a comment

lucksus Jul 17, 2019

lucksus Jul 17, 2019

lucksus Jul 17, 2019

Get links Count #1568

Get links Count #1568

Conversation

StaticallyTypedAnxiety commented Jul 2, 2019

PR summary

testing/benchmarking notes

followups

changelog

StaticallyTypedAnxiety commented Jul 2, 2019 • edited Loading

StaticallyTypedAnxiety commented Jul 2, 2019

lucksus left a comment

Choose a reason for hiding this comment

lucksus Jul 12, 2019

Choose a reason for hiding this comment

StaticallyTypedAnxiety Jul 15, 2019

Choose a reason for hiding this comment

zippy left a comment

Choose a reason for hiding this comment

zippy Jul 5, 2019

Choose a reason for hiding this comment

StaticallyTypedAnxiety Jul 15, 2019

Choose a reason for hiding this comment

lucksus left a comment

Choose a reason for hiding this comment

lucksus Jul 17, 2019

Choose a reason for hiding this comment

lucksus Jul 17, 2019

Choose a reason for hiding this comment

lucksus Jul 17, 2019

Choose a reason for hiding this comment

StaticallyTypedAnxiety commented Jul 2, 2019 •

edited

Loading