[WIP] Sub graphs and Decorate #240

srikrsna · 2019-04-30T11:02:33Z

Also address #230

Added support for creating child containers.

Containers have access to all types provided using their parents (Invoke works anywhere)
Containers have access to all types provided using their children (Invoke works anywhere)
Provide checks the entire graph (all containers both up and down) for duplicates
All success tests repeated for children
Tests for multiple and multi level children
Test for duplicate check in one of the children
Stringer updated

codecov · 2019-04-30T11:04:56Z

Codecov Report

❗ No coverage uploaded for pull request base (master@4cc9f88). Click here to learn what that means.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##             master     #240   +/-   ##
=========================================
  Coverage          ?   98.51%           
=========================================
  Files             ?       10           
  Lines             ?     1210           
  Branches          ?        0           
=========================================
  Hits              ?     1192           
  Misses            ?       13           
  Partials          ?        5

Impacted Files	Coverage Δ
dig.go	`100.00% <100.00%> (ø)`
stringer.go	`100.00% <100.00%> (ø)`

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 4cc9f88...d0426c0. Read the comment docs.

srikrsna · 2019-04-30T12:37:19Z

@abhinav

I've added supported for sub graphs. Please review and provide feedback. Once this is approved I'll proceed to implementing Decorate

Also how should I implement Stringer on a container. Should it also print all the nodes of the parents? or all the nodes of children? or both?

I would go for both since it would be consistent with the behaviour. But I'm not sure.

abhinav

Thanks for the change @srikrsna! The general direction this is taking is
looking great. I've left a few suggestions.

dig.go

dig_test.go

Co-Authored-By: srikrsna <psk.psk6@gmail.com>

abhinav

Thanks for making the changes @srikrsna. I think we still want to de-duplicate the tests as suggested in https://github.com/uber-go/dig/pull/240/files#r279888001

srikrsna · 2019-05-03T07:43:03Z

@abhinav I've updated the tests. One more thing I needed direction on is how should the Stringer method be updated. Should it also print all the nodes of the parents? or all the nodes of children? or both?

I would go for both since it would be consistent with the behaviour of Invoke and Provide. But I'm not sure.

abhinav · 2019-05-06T17:21:51Z

Hey, sorry for the delay. That's a fair point. It might be a bit surprising if
String() prints only 5 types but the container has access to 50. That said,
the expectation is still for String() to print a representation of the
object under inspection, even if has knowledge of more.

So I think it's best if String only print its own nodes and recurses into
children. It could also print the address of the parent (if any) so we know
that part of the graph is hidden.

Rather than a function which returns two other functions, this changes the container hierachy tests to consume a `containerView` which has `Provide` and `Invoke` methods as fields on it. This has the effect of leaving the bulk of the test logic unchanged, only changing `New()` calls to `newContainer()`.

abhinav · 2019-05-06T17:36:37Z

I made a minor simplification to the tests. Please pull that before you make
more changes.

srikrsna · 2019-05-06T18:00:59Z

I'll update the String() method to print all its children and also the parent if any. Also, can I proceed to implement Decorate?

abhinav · 2019-05-06T18:14:17Z

I'll update the String() method to print all its children and also the parent if any.

Just the address of the parent instead of the contents might be less noisy and would help differentiate between parent and child in the output.

Also, can I proceed to implement Decorate?

Yes, definitely! Although I would make that a separate PR on top of this one so that this can be landed when ready and that can be iterated on independently.

srikrsna · 2019-05-28T04:58:03Z

@abhinav Apologies for the delay. I believe this is complete and ready to be merged.

abhinav · 2019-06-07T18:46:18Z

Hey @srikrsna! Thanks! We haven't forgotten about this but we're a bit swamped
right now. I'll try to get into this as soon as possible.

CLAassistant · 2020-06-13T13:15:08Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ abhinav
❌ Sri Krishna Paritala

Sri Krishna Paritala seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

ryus08 · 2020-06-13T19:25:10Z

I know this is pretty darn old at this point, but FWIW, is a type of feature I personally look for in an IoC container, and seemed pretty close to good to go last year.

jstroem · 2020-11-23T07:33:39Z

This is what I awesome! Have been waiting for this feature..
@srikrsna seems that this is almost done, will you be willing to sign the CLA?

srikrsna · 2020-11-23T08:22:26Z

Done

jstroem · 2020-11-23T13:35:01Z

@abhinav any way you can push on this?

yiminc · 2021-09-15T04:00:10Z

@srikrsna @abhinav why not merge this PR?

abhinav · 2021-09-16T19:02:32Z

Hey folks, apologies for the radio silence here.

I know it's been a while but we're trying to pick up support for decorate/replace again,
so we'll try to update the relevant tasks as soon as we have something.

That said, the reason this PR was not merged was this:
we started reconsidering whether subgraphs were the best way to implement this feature.

Unfortunately, we didn't write down those conversations, but from what I can recall:
We didn't like that the semantics of subgraphs were not super clear.
I'm the one who suggested those semantics, and I still couldn't tell you off the top of my head
whether a subgraph has access to everything provided by a parent and everything provided by a sibling.
That it does, based on the name subgraph, is surprising behavior.

So we weren't ready to commit to subgraphs at the time,
and then the project just got buried under other priorities.

All's not lost, though. As I said, we're trying to pick up the project again.
We'll try to update the relevant tasks (#230 and uber-go/fx#653) as soon as we have something.

Sri Krishna Paritala added 2 commits April 30, 2019 16:29

Initial support for sub graphs

2d7a27e

Comments for Container.Child

dceb621

Sri Krishna Paritala added 5 commits April 30, 2019 17:13

Added Tests

24f7ecd

Added Tests for Group operations

59348c5

Removed: panic if name is empty

4546ac5

Added tests for provide failures

364da8c

Remove redundant check in provide

66b0bb1

srikrsna changed the title ~~[WIP] Support sub graphs~~ [WIP] Support sub graphs and Decorate Apr 30, 2019

srikrsna changed the title ~~[WIP] Support sub graphs and Decorate~~ [WIP] Sub graphs and Decorate Apr 30, 2019

abhinav reviewed Apr 30, 2019

View reviewed changes

Apply suggestions from code review

f91df49

Co-Authored-By: srikrsna <psk.psk6@gmail.com>

abhinav reviewed May 1, 2019

View reviewed changes

Removed duplicate tests. Added tests for cases where sub graphs can fail

e83edb4

Update Stringer Implementation

d0426c0

srikrsna closed this Jun 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Sub graphs and Decorate #240

[WIP] Sub graphs and Decorate #240

srikrsna commented Apr 30, 2019 •

edited

Loading

codecov bot commented Apr 30, 2019 •

edited

Loading

srikrsna commented Apr 30, 2019 •

edited

Loading

abhinav left a comment

abhinav left a comment

srikrsna commented May 3, 2019

abhinav commented May 6, 2019

abhinav commented May 6, 2019

srikrsna commented May 6, 2019

abhinav commented May 6, 2019

srikrsna commented May 28, 2019

abhinav commented Jun 7, 2019

CLAassistant commented Jun 13, 2020 •

edited

Loading

ryus08 commented Jun 13, 2020

jstroem commented Nov 23, 2020

srikrsna commented Nov 23, 2020

jstroem commented Nov 23, 2020 •

edited

Loading

yiminc commented Sep 15, 2021

abhinav commented Sep 16, 2021

[WIP] Sub graphs and Decorate #240

[WIP] Sub graphs and Decorate #240

Conversation

srikrsna commented Apr 30, 2019 • edited Loading

codecov bot commented Apr 30, 2019 • edited Loading

Codecov Report

srikrsna commented Apr 30, 2019 • edited Loading

abhinav left a comment

Choose a reason for hiding this comment

abhinav left a comment

Choose a reason for hiding this comment

srikrsna commented May 3, 2019

abhinav commented May 6, 2019

abhinav commented May 6, 2019

srikrsna commented May 6, 2019

abhinav commented May 6, 2019

srikrsna commented May 28, 2019

abhinav commented Jun 7, 2019

CLAassistant commented Jun 13, 2020 • edited Loading

ryus08 commented Jun 13, 2020

jstroem commented Nov 23, 2020

srikrsna commented Nov 23, 2020

jstroem commented Nov 23, 2020 • edited Loading

yiminc commented Sep 15, 2021

abhinav commented Sep 16, 2021

srikrsna commented Apr 30, 2019 •

edited

Loading

codecov bot commented Apr 30, 2019 •

edited

Loading

srikrsna commented Apr 30, 2019 •

edited

Loading

CLAassistant commented Jun 13, 2020 •

edited

Loading

jstroem commented Nov 23, 2020 •

edited

Loading