Skip to content

Conversation

@awrichar
Copy link
Contributor

No description provided.

Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
@codecov-commenter
Copy link

Codecov Report

Merging #446 (468b59e) into main (4bc6c91) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #446   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          278       279    +1     
  Lines        14981     14995   +14     
=========================================
+ Hits         14981     14995   +14     
Impacted Files Coverage Δ
internal/orchestrator/orchestrator.go 100.00% <ø> (ø)
...ternal/apiserver/route_get_txn_blockchainevents.go 100.00% <100.00%> (ø)
internal/apiserver/route_get_txn_ops.go 100.00% <100.00%> (ø)
internal/orchestrator/data_query.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 4bc6c91...468b59e. Read the comment docs.

Copy link
Contributor

@peterbroadhurst peterbroadhurst left a comment

Choose a reason for hiding this comment

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

One trivial suggestion, with approval.

FilterFactory: nil,
Description: i18n.MsgTBD,
JSONInputValue: nil,
JSONOutputValue: func() interface{} { return &[]*fftypes.BlockchainEvent{} },
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't need the & here I think as arrays are a reference type, although it doesn't really matter.

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 copy-pasted this, but your comment prompted me to do a quick investigation... it actually looks like the Swagger generation for the output schema is only correct if you do include the &. So for this route it correctly indicates that it will return an array of BlockchainEvent. But for other routes that don't return a pointer, it's a bit broken - such as for /transactions, which returns []*fftypes.Transaction, the Swagger indicates it will return a single Transaction object.

I take this to mean we should add the & everywhere on array returns, or investigate why the Swagger generation behaves this way.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for digging! Will merge based on this and raise an issue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I see #449

@peterbroadhurst peterbroadhurst merged commit 0ab0206 into hyperledger:main Jan 26, 2022
@peterbroadhurst peterbroadhurst deleted the blockchainevents branch January 26, 2022 16:55
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