Skip to content
This repository was archived by the owner on Nov 5, 2025. It is now read-only.

Comments

Resolve reviews for #145#149

Merged
graywolf336 merged 5 commits intoRocketChat:alphafrom
shiqimei:draft-resolve-reviews
Oct 2, 2019
Merged

Resolve reviews for #145#149
graywolf336 merged 5 commits intoRocketChat:alphafrom
shiqimei:draft-resolve-reviews

Conversation

@shiqimei
Copy link
Contributor

@shiqimei shiqimei commented Oct 2, 2019

  • remove tests externalComponentBridge

@codecov
Copy link

codecov bot commented Oct 2, 2019

Codecov Report

Merging #149 into alpha will decrease coverage by 1.97%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##            alpha     #149      +/-   ##
==========================================
- Coverage   59.65%   57.67%   -1.98%     
==========================================
  Files          73       65       -8     
  Lines        2377     2266     -111     
  Branches      350      350              
==========================================
- Hits         1418     1307     -111     
  Misses        959      959
Impacted Files Coverage Δ
src/server/managers/AppExternalComponentManager.ts 20% <ø> (ø) ⬆️
src/server/accessors/ConfigurationExtend.ts 100% <100%> (ø) ⬆️
src/server/accessors/ExternalComponentExtend.ts 50% <100%> (ø)
src/server/managers/AppAccessorManager.ts 100% <100%> (ø) ⬆️
src/server/errors/index.ts
src/server/managers/index.ts
src/server/marketplace/license/index.ts
src/server/compiler/index.ts
src/server/storage/index.ts
src/server/logging/index.ts
... and 1 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 b3f6979...d444646. Read the comment docs.

* rename IExternalComponentsExtend to IExternalComponentExtend
* rename externalComponents to externalComponent
Copy link
Contributor

@graywolf336 graywolf336 left a comment

Choose a reason for hiding this comment

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

While I approve of these changes and making them consistent, I am still not sure I understand the single external component requirement. Why are we limiting Apps to just a single external component? (let's move this conversation to the other pull request)

@graywolf336 graywolf336 merged commit b21d172 into RocketChat:alpha Oct 2, 2019
@shiqimei shiqimei deleted the draft-resolve-reviews branch October 2, 2019 16:50
@shiqimei
Copy link
Contributor Author

shiqimei commented Oct 2, 2019

While I approve of these changes and making them consistent, I am still not sure I understand the single external component requirement. Why are we limiting Apps to just a single external component? (let's move this conversation to the other pull request)

Please see #145 (comment) @graywolf336 . Some of my ideas are there.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants