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

chore: adding @returns for provideOrchestration #9803

Closed
wants to merge 1 commit into from

Conversation

amessbee
Copy link
Contributor

Adding a @returns tag for provideOrchestration API

refs:
#9801
#9757

Description

@dckc
I am adding a @returns tag for provideOrchestration API based on what I see in code - if something is wrong, please make suggestions.

Security/Scaling/Documentation Considerations

None

Testing Considerations

Manual

Upgrade Considerations

May need to be updated if provideOrchestration changes.

@amessbee amessbee requested review from turadg and dckc July 30, 2024 10:11
Copy link
Member

@turadg turadg left a comment

Choose a reason for hiding this comment

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

Before approving I need to understand whether we really need #9801

Also, for this specific one, @dtribble has suggested we get rid of provideOrchestration and have just withOrchestration. Let's settle that before putting more effort into the provideOrchestration signature.

@@ -43,6 +43,8 @@ import { makeZoeTools } from './zoe-tools.js';
* @param {Baggage} baggage
* @param {OrchestrationPowers} remotePowers
* @param {Marshaller} marshaller
* @returns {(facade: OrchestrationFacade, chainHub: ChainHub, vowTools: VowTools, zoeTools: ZoeTools, zone: Zone} ochestration
Copy link
Member

Choose a reason for hiding this comment

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

Why? This function is meant to return whatever it does so putting this here will require updating two places each time.

As motivation you referenced #9801 which says,

needed for clarity and documentation

Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In general, many orchestration API lack @returns. Mine is a newbie (in blockchain and at Agoric) dev perspective, that I would be more confident using an API where return signature is clearly defined in docs, and I don't have to go through the code. Again, I am probably wrong, but this is how I see it.

On a side note, for my learning, what would be a potentially satisfactory answer to your why question?

Copy link
Member

Choose a reason for hiding this comment

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

satisfactory answer

Something showing that the change is in fact needed for clarity and documentation. I assume we're talking about the docsite documentation and clarity for the reader of that.

I went to see how this changes provideOrchestration in its output but it's not in there at all. We need to export it in the top-level index.js.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since, provideOrchestration is to be marked @internal (relevant discussion in slack) there is no need for this PR, so I am closing this.

Copy link

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 366a7be
Status:🚫  Build failed.

View logs

@amessbee amessbee closed this Jul 31, 2024
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.

2 participants