application-services icon indicating copy to clipboard operation
application-services copied to clipboard

Adding twin ADRS on wrapper code and async Rust

Open bendk opened this issue 2 years ago • 3 comments

These are independant but related decisions, so two ADRs in one pull request is a good way to handle them.

Pull Request checklist

  • Breaking changes: This PR follows our breaking change policy
    • [ ] This PR follows the breaking change policy:
      • This PR has no breaking API changes, or
      • There are corresponding PRs for our consumer applications that resolve the breaking changes and have been approved
  • [ ] Quality: This PR builds and tests run cleanly
    • Note:
      • For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
      • If this pull request includes a breaking change, consider cutting a new release after merging.
  • [ ] Tests: This PR includes thorough tests or an explanation of why it does not
  • [ ] Changelog: This PR includes a changelog entry in CHANGELOG.md or an explanation of why it does not need one
    • Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
  • [ ] Dependencies: This PR follows our dependency management guidelines
    • Any new dependencies are accompanied by a summary of the due dilligence applied in selecting them.

Branch builds: add [firefox-android: branch-name] to the PR title.

bendk avatar Nov 09 '23 17:11 bendk

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (8aa85ff) 36.68% compared to head (ae48436) 36.68%. Report is 4 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5910   +/-   ##
=======================================
  Coverage   36.68%   36.68%           
=======================================
  Files         348      348           
  Lines       33663    33663           
=======================================
  Hits        12349    12349           
  Misses      21314    21314           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Nov 09 '23 17:11 codecov-commenter

For Async Rust, it's too early to commit to embracing it but I would love to see a small component running using Async Rust as support.

I wouldn't want to move faster than that either. I think calling the strategy "embracing Async Rust" is too much, it should be something more like "timidly waving at Async Rust from 100ft away".

bendk avatar Nov 09 '23 23:11 bendk

Thanks for all the feedback here. I just pushed an update with a bunch of changes, the main ones are:

  • Significantly re-framed the wrapper questions. Instead of "do we like wrappers or not?", it's now "how much effort are we willing to put into removing wrappers?"
  • Added an option (C) for the async question

bendk avatar Nov 13 '23 16:11 bendk