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
Make SnapController.add()
private
#883
Conversation
@@ -1780,7 +1766,7 @@ export class SnapController extends BaseController< | |||
* version. | |||
* @returns The resulting snap object. | |||
*/ | |||
async add(args: AddSnapArgs): Promise<PersistedSnap> { | |||
private async _add(args: AddSnapArgs): Promise<PersistedSnap> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we truly make it private?
private async _add(args: AddSnapArgs): Promise<PersistedSnap> { | |
async #add(args: AddSnapArgs): Promise<PersistedSnap> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While we're at it, I really dislike having _
in names in general, I think the SnapController
is the only place we do it. Might as well just make them normal names but have a private modifier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Mrtenz Probably, but that would require more test refactoring than I am strictly willing to tackle right now 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ritave Yeah that's a good point, we can address that in a follow-up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you make an issue about it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to merge upon approval. |
2fb2676
to
7232f27
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imo this is good to go - @Mrtenz can you take another look
This PR turns
SnapController.add()
into a private method_add()
and removes its associated action. Due to the extensive use of this method in tests, a newSnapController
persisted state factory is added to our test utilities and used instead ofadd()
. Overall unit tests coverage has decreased slightly due to the removal of tested code.