Skip to content

Conversation

@bramkragten
Copy link
Member

@bramkragten bramkragten commented Oct 1, 2023

Summary

Adds a confirmation for call_service, fire_event and send_location, render_template url action.

Screenshots

Link to pull request in Documentation repository

Documentation: home-assistant/companion.home-assistant#

Any other notes

@codecov
Copy link

codecov bot commented Oct 1, 2023

Codecov Report

Merging #2417 (eea2d8f) into master (7c55eb6) will decrease coverage by 0.08%.
The diff coverage is 2.87%.

@@            Coverage Diff             @@
##           master    #2417      +/-   ##
==========================================
- Coverage   28.30%   28.22%   -0.08%     
==========================================
  Files         273      273              
  Lines       30077    30160      +83     
==========================================
  Hits         8514     8514              
- Misses      21563    21646      +83     
Files Coverage Δ
Sources/App/WebView/IncomingURLHandler.swift 3.13% <2.87%> (-0.66%) ⬇️

@bgoncal
Copy link
Member

bgoncal commented Oct 2, 2023

Besides missing tests (not sure how's the criteria we use for that) it looks good to me 👍🏻

}
}

private func confirmAction(title: String, message: String, handler: @escaping () -> Void, cancelHandler: (() -> Void)? = nil) {
Copy link
Member

Choose a reason for hiding this comment

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

-> Promise<Void> may make the call sites a little easier, e.g.

confirmAction().done {
  // do the thing
}.catch {
  // do an error
}

and .cauterize() when we don't care about the error, or in the places right now…

firstly {
  confirmAction()
}.done {
  // rest of the chains
}

@zacwest
Copy link
Member

zacwest commented Oct 2, 2023

you'll want to run the linting scripts to make that check happy, at least.

code coverage for some of these areas is missing.

@zacwest zacwest merged commit 8eaaaac into home-assistant:master Oct 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants