[RFC - Server] Refining the service builder API #1859

LukeMathWalker · 2022-10-17T07:35:30Z

Motivation and Context

This PR adds an RFC that explores how we could evolve the new service builder API to improve developer experience, in particular for Rust beginners.

Rendered RFC

The API proposed in this RFC has been manually implemented for the Pokemon service. You can find the code here.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

github-actions · 2022-10-17T07:51:19Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
No codegen difference in the Server Test
No codegen difference in the Server Test Python

A new doc preview is ready to view.

jjant · 2022-10-17T09:13:57Z

design/src/rfcs/rfc0022_refine_builder.md

+- com.aws.example#CheckHealth
+
+Use the dedicated methods on `PokemonServiceBuilder` to register the missing handlers:
+- PokemonServiceBuilder::check_health


Beginners to rust may not know that this is telling them to use builder.check_health(), maybe we should clarify that.

design/src/rfcs/rfc0022_refine_builder.md

jjant · 2022-10-17T09:26:16Z

design/src/rfcs/rfc0022_refine_builder.md

+```rust
+pub struct PokemonServiceBuilder<
+    Op1,
+    Op2,
+    Op3,
+    Op4,
+    Op5,
+    Op6,
+    Exts1 = (),
+    Exts2 = (),
+    Exts3 = (),
+    Exts4 = (),
+    Exts5 = (),
+    Exts6 = (),
+    Pl = aws_smithy_http_server::plugin::IdentityPlugin,
+> {
+    check_health: Op1,
+    do_nothing: Op2,
+    get_pokemon_species: Op3,
+    get_server_statistics: Op4,
+    capture_pokemon: Op5,
+    get_storage: Op6,
+    #[allow(unused_parens)]
+    _exts: std::marker::PhantomData<(Exts1, Exts2, Exts3, Exts4, Exts5, Exts6)>,
+    plugin: Pl,
+}
+```
+
+to:
+
+```rust
+pub struct PokemonServiceBuilder<
+    Op1,
+    Op2,
+    Op3,
+    Op4,
+    Op5,
+    Op6,
+    Exts1 = (),
+    Exts2 = (),
+    Exts3 = (),
+    Exts4 = (),
+    Exts5 = (),
+    Exts6 = (),
+    Pl = aws_smithy_http_server::plugin::IdentityPlugin,
+> {
+    check_health: Option<Op1>,
+    do_nothing: Option<Op2>,
+    get_pokemon_species: Option<Op3>,
+    get_server_statistics: Option<Op4>,
+    capture_pokemon: Option<Op5>,
+    get_storage: Option<Op6>,
+    #[allow(unused_parens)]
+    _exts: std::marker::PhantomData<(Exts1, Exts2, Exts3, Exts4, Exts5, Exts6)>,
+    plugin: Pl,
+}
+```


Maybe a diff view would make the distinction clearer here (tho you lose syntax highlighting AFAIK).

pub struct PokemonServiceBuilder< Op1, Op2, Op3, Op4, Op5, Op6, Exts1 = (), Exts2 = (), Exts3 = (), Exts4 = (), Exts5 = (), Exts6 = (), Pl = aws_smithy_http_server::plugin::IdentityPlugin, > { - check_health: Op1, + check_health: Option<Op1>, ... #[allow(unused_parens)] _exts: std::marker::PhantomData<(Exts1, Exts2, Exts3, Exts4, Exts5, Exts6)>, plugin: Pl, }

hlbarber

Looking good so far.

I think we should include a section on what happens to FailOnMissingOperation etc. There are various use cases where we want to opt out of the normal Operation<S, L> upgrade procedure.

design/src/rfcs/rfc0022_refine_builder.md

hlbarber · 2022-10-17T10:27:13Z

design/src/rfcs/rfc0022_refine_builder.md

+
+Trait objects are another common approach to achieve type erasure: instead of storing the raw handler in `PokemonServiceBuilder`, we box it and cast it to a `dyn Upgradable<...>`.  
+Using trait objects would allow us to delay upgrading to `Route` until `build` is called, mimicking the behaviour of the current API and allowing developers to register plugins at any point before calling `build`.  
+Using trait objects would unfortunately have a non-zero runtime impact: we would be introducing an extra layer of indirection (double-boxing the handler), affecting the performance of our application at runtime.  


I don't think this is the case here - we could Box<Box<Box<dyn Upgradable<...>>>> but the actual upgrade procedure would still result in a singly nested handler (Route::new). So the boxed trait objects here would affect startup but not application runtime.

If we were to type erase via Route::new we would get double-boxed handlers.

I don't think this is the case here - we could Box<Box<Box<dyn Upgradable<...>>>> but the actual upgrade procedure would still result in a singly nested handler (Route::new)

Is this the case though?

Route::new desugars to a BoxCloneService::new call, which introduces one layer of vtable indirection.
The argument to BoxCloneService::new is the parameter passed to Route::new - if that were to be behind multiple layers of boxing, I am under the impression that we would have to resolve those indirections at runtime when invoking the service.

It's a different situation when we are talking about boxed Plugins - the call to .map would suffer from the indirection, but that only happens at startup time.

Have I got this right or am I missing something (more than likely, tbh)?

Currently, Upgradable looks like this:

/// Provides an interface to convert a representation of an operation to a HTTP [`Service`](tower::Service) with /// canonical associated types. pub trait Upgradable<Protocol, Operation, Exts, B, Plugin> { type Service: Service<http::Request<B>, Response = http::Response<BoxBody>>; /// Performs an upgrade from a representation of an operation to a HTTP [`Service`](tower::Service). fn upgrade(self, plugin: &Plugin) -> Self::Service; }

If we wanted to make the type uniform we'd have to remove the associated type which would vary across each operation (else we'd have to specify it in the dyn Upgradable<Service = ....>). So we'd have to move the Route::new into the Ugpradable, like this:

pub trait Upgradable<Protocol, Operation, Exts, B, Plugin> { fn upgrade(self, plugin: &Plugin) -> Route<B>; }

I think is acceptable tweak - all the gnarly Send + Clone + ... bounds would be pushed down into the Upgradable for Operation impl too, rather than hanging around on the service builder which might be worth while anyway (I recall you mentioning this before).

The .boxed() method on the service builder would then take the OpX and turn it into a Box<dyn Upgradable<...>>. The Box here would box the dyn Upgradable and if we call .boxed() multiple times it'd become Box<Box<Box<dyn Upgradable>>> but this is Boxing the actual dyn Upgrade, rather than creating a Box<dyn Service>? The Route::new is still only called once and hence the indirection at runtime only happens once as far as I can tell? The way I'm imagining it is that Box<Box<Box<dyn FnOnce() -> Box<T>>>> when called returns a Box<T> whose access requires 1 indirection rather than 3.

hlbarber · 2022-10-17T10:29:07Z

design/src/rfcs/rfc0022_refine_builder.md

+```rust
+// Currently you'd have to go for `PluginStack::new(IdentityPlugin, IdentityPlugin)`,
+// but that can be smoothed out even if this RFC isn't approved.
+let plugin_stack = PluginStack::default()


There's an alternative approach we could also take here which gives us some different dynamics.

Currently, because the "upgrade" is performed in the build method, all applied Plugins are used in build. This means that

.get_pokemon_species(/* handler */) .get_storage(/* handler */) .print() .build()

is equivalent to

.get_pokemon_species(/* handler */) .print() .get_storage(/* handler */) .build()

If we are eagerly Route::newing in the setters we could have the current Plugin be used. Which means that the two snippets above would not be equivalent - the latter would not apply the print plugin to the get_storage handler.

This would allow the customer to "scope" plugins in a coarse way - I've been asked if this is the current behavior. I'm not convinced this is a good approach - it might cause confusion.

I should have called this out in the RFC, but I am indeed not convinced this is the right way forward for the same reason you mentioned - it can be confusing and lead to surprising runtime behaviour.
Especially considering how many existing web frameworks in Rust ignore at what point a middleware is registered and apply it to all routes at the same scope/level.

If the intention is that the plugin would only affect the operations registered so far, I think it's ok.

We can also consider providing a way to apply plugins (i.e. layers to the operation before it's HTTP-upgraded) more fine-grained, to specific operations via a method on Operation, instead of the current way in #1837. But this is a one-way door, we can add this functionality later if there's user interest.

We can also consider providing a way to apply plugins (i.e. layers to the operation before it's HTTP-upgraded) more fine-grained, to specific operations via a method on Operation, instead of the current way in #1837.

I think just adding a blanket impl

impl<P, Op, Pl, S, L> Pluggable<Pl> for Operation<S, L> where Pl: Plugin<P, Op, S, L> { type Output = Operation<Pl::Service, Pl::Layer>; fn apply(self, new_plugin: Pl) -> Self::Output { new_plugin.map(self) } }

to the aws-smithy-http-server would allow you to

op.print()

where op: Operation.

I have captured both as an alternative in 90ae7aa
I've added as well a sketch of an API for attaching plugins to groups of routes using a nested/scoped builder.

hlbarber · 2022-10-17T10:49:00Z

design/src/rfcs/rfc0022_refine_builder.md

+It supports fine-grained configuration at multiple levels (per-handler middlewares, router middlewares, plugins) while trying to prevent some misconfiguration issues at compile-time (i.e. missing operation handlers).  
+There is consensus that the new API is an improvement over the pre-existing `OperationRegistryBuilder`/`OperationRegistry`, which is now on its way to deprecation in one of the next releases.  
+
+This RFC builds on top of [RFC 20] to explore an alternative API design prior to its stabilisation.  


I think it'd be nice to link to the exact section which we aim to revert here.

https://github.com/awslabs/smithy-rs/blob/main/design/src/rfcs/rfc0020_service_builder.md#statically-check-for-missing-handlers

I haven't linked a specific section because this is not technically a "revert" - we had those generic parameters in the OperationRegistryBuilder version as well, so reverting the compile-safety piece does not lead us to a less generic API per se.

hlbarber · 2022-10-17T10:51:53Z

design/src/rfcs/rfc0022_refine_builder.md

+
+### Alternatives: boxed trait objects 
+
+Trait objects are another common approach to achieve type erasure: instead of storing the raw handler in `PokemonServiceBuilder`, we box it and cast it to a `dyn Upgradable<...>`.  


Perhaps a little example of using the type erasure method e.g. boxed with a conditional branch?

You could note a downside here that we cannot type erase MissingOperation etc this way, so the branches would still have the MissingOperation stuck in their signature.

design/src/rfcs/rfc0022_refine_builder.md

hlbarber · 2022-10-17T11:02:55Z

design/src/rfcs/rfc0022_refine_builder.md

+```rust
+impl PokemonService<()> {
+    /// Constructs a builder for [`PokemonService`].
+    pub fn builder<Body, Plugin>(plugin: Plugin) -> PokemonServiceBuilder<Body, Plugin> {


We could also do a two-phase builder which keeps the user in the method chain, something like:

let app = PokemonService::builder() // : PokemonServicePluginBuilder<IdentityPlugin> .print() /* ... */ .finalize_plugins() // PokemonServiceBuilder<B, PluginStack<IdentityPlugin, ..., PrintPlugin>> .get_pokemon_species(/* handler */) ... .build();

Yeah, this could work as well, with a PokemonService::builder_without_plugins() for the case where the developer doesn't want to register any plugin and PokemonService::builder().finalize_plugins() would be ugly.

hlbarber · 2022-10-17T11:13:10Z

design/src/rfcs/rfc0022_refine_builder.md

+```rust
+impl PokemonService<()> {
+    /// Constructs a builder for [`PokemonService`].
+    pub fn builder<Body, Plugin>(plugin: Plugin) -> PokemonServiceBuilder<Body, Plugin> {


I think it's worth noting that now Plugin might be a complicated type given that it must be fully specified up front. Do we need to type erase it?

I do expect that it should be possible to simply have an unconstrained Plugin generic parameter in most functions where you pass the builder around, but we should have a .boxed() method to type-erase the plugin if necessary.
It feels like it won't be very amenable to an impl Trait approach in function signatures.

hlbarber · 2022-10-17T11:29:02Z

design/src/rfcs/rfc0022_refine_builder.md

+```rust
+// Currently you'd have to go for `PluginStack::new(IdentityPlugin, IdentityPlugin)`,
+// but that can be smoothed out even if this RFC isn't approved.
+let plugin_stack = PluginStack::default()


I think it'd also be a good idea to make a PluginBuilder here - PluginStack is kinda confusing term to be a primary API.

This would mirror how tower has a ServiceBuilder and Stack.

Captured this in a9aab96 👍🏻

hlbarber · 2022-10-17T11:29:36Z

design/src/rfcs/rfc0022_refine_builder.md

+```
+
+This pattern needs to be revisited if we want to move forward with this RFC, since new plugins cannot be registered after the builder has been instantiated.  
+My recommendation would be to implement `Pluggable` for `PluginStack`, providing the same pattern ahead of the creation of the builder:


I think this is a great bit of luck that this migrates so cleanly. There's no harm in doing this kind of impl even without this RFC being accepted.

david-perez

I don't think the missing handler error message is persuasive; there are sharper edges elsewhere. The other two identified shortcomings (branching and partial_setup) are very compelling and I agree we should do something to remediate them.

david-perez · 2022-10-17T11:27:33Z

design/src/rfcs/rfc0022_refine_builder.md

+Let's try to register an operation handler now:
+
+```rust
+fn partial_setup<Op1, Op2, Op3, Op4, Op5, Op6>(


Far from ideal, but for simple use cases, you could get away with a macro that performs this initialization.

david-perez · 2022-10-17T11:38:46Z

design/src/rfcs/rfc0022_refine_builder.md

+error[E0277]: the trait bound `MissingOperation: Upgradable<AwsRestJson1, CheckHealth, (), _, IdentityPlugin>` is not satisfied
+  --> pokemon-service/src/bin/pokemon-service.rs:38:10
+   |
+38 |         .build();
+   |          ^^^^^ the trait `Upgradable<AwsRestJson1, CheckHealth, (), _, IdentityPlugin>` is not implemented for `MissingOperation`
+   |
+   = help: the following other types implement trait `Upgradable<Protocol, Operation, Exts, B, Plugin>`:
+             FailOnMissingOperation
+             Operation<S, L>


I actually think this is one of the nicer error messages you can get. It has enough of the right words MissingOperation, CheckHealth for it to likely to not cause much confusion.

The error messages you get when passing in a handler that does not satisfy the type signature (not async, wrong input / output types) are worse, although those (and perhaps this one too?) can be palliated with a debug macro à la axum's debug_handler, which I think we direly need anyway.

I expect this error to not be frequent. Users will likely start off from a template; the new service builder needs to generate Rust docs with an out-of-the-box compilable program, like in #1498 (comment).

The error messages you get when passing in a handler that does not satisfy the type signature (not async, wrong input / output types) are worse, although those (and perhaps this one too?) can be palliated with a debug macro à la axum's debug_handler, which I think we direly need anyway.

I don't disagree that those errors are worse, but I don't think that the builder case can be fixed using a debug macro. Or, at least, not in the general case nor easily.
The macro would need to wrap the entire scope where the builder is being used, therefore ruling out refactorings that break the startup logic across multiple functions. We would also have to understand branches in the code we are wrapping to make sure that all paths lead to a correctly instantiated builder - it gets nasty quite quickly.

Yeah it's not feasible.

We could improve the error messages if we named the type params OpUnset, OpSet etc https://github.com/estebank/makeit#error-messages

david-perez · 2022-10-17T11:43:14Z

design/src/rfcs/rfc0022_refine_builder.md

+
+Trait objects are another common approach to achieve type erasure: instead of storing the raw handler in `PokemonServiceBuilder`, we box it and cast it to a `dyn Upgradable<...>`.  
+Using trait objects would allow us to delay upgrading to `Route` until `build` is called, mimicking the behaviour of the current API and allowing developers to register plugins at any point before calling `build`.  
+Using trait objects would unfortunately have a non-zero runtime impact: we would be introducing an extra layer of indirection (double-boxing the handler), affecting the performance of our application at runtime.  


I think this is worth exploring. Following a vtable has negligible performance impact for our use cases. If you can flatten the type that the router ends up holding to just a few boxes, it's likely ok.

I've significantly expanded this section exploring two different approaches for lazy type erasure.
I don't believe it's actually practical moving forward with either of them - they do improve slightly on the current status quo but they carry a significant complexity budget in terms of implementation and ongoing maintenance.
We spent some time we @hlbarber to play around with ideas and they get quite complex quite quickly.

github-actions · 2022-10-17T15:34:24Z

A new generated diff is ready to view.

A new doc preview is ready to view.

github-actions · 2022-10-17T16:10:13Z

A new generated diff is ready to view.

A new doc preview is ready to view.

github-actions · 2022-10-17T16:56:15Z

A new generated diff is ready to view.

A new doc preview is ready to view.

github-actions · 2022-10-18T15:36:02Z

A new generated diff is ready to view.

A new doc preview is ready to view.

github-actions · 2022-10-19T13:21:57Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
No codegen difference in the Server Test
No codegen difference in the Server Test Python

A new doc preview is ready to view.

github-actions · 2022-10-21T15:26:38Z

A new generated diff is ready to view.

A new doc preview is ready to view.

github-actions · 2022-10-21T16:02:59Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
No codegen difference in the Server Test
No codegen difference in the Server Test Python

A new doc preview is ready to view.

hlbarber · 2022-10-24T10:44:48Z

design/src/rfcs/rfc0022_refine_builder.md

+Having to specify all plugins upfront is unlikely to have a negative impact on developers currently using `smithy-rs`.
+We have seen how cumbersome it is to break the startup logic into different functions using the current service builder API. Developers are most likely specifying all plugins and routes in the same function even if the current API allows them to intersperse route registrations and plugin registrations: they would simply have to re-order their registration statements to adopt the API proposed in this RFC.
+
+### Alternatives: allow new plugins to be registered after builder creation


I think we should add an additional alternative here:

Don't solve the function signature problem at all, the customer just has to PokemonService::builder()....build() within one function.

Solve the conditional branch problem with Either<A, B> types (e.g. A: Upgradable<...> and B: Upgradable<...> implies Either<A, B>: Upgradable.

Don't solve the function signature problem at all, the customer just has to PokemonService::builder()....build() within one function.

I think the RFC should mention using macros instead of functions to workaround this limitation.

Macros that we provide or macros written on an ad-hoc basis by the customer?

Written by the user. The problem here is the user having to type the complicated function signature that does partial initialization. If they just plop the function body into a macro, they don't have to.

I'd argue that writing a macro is a skill outside the comfort zone of most Rust users, who primarily consume them. Sketching a very simple macro_rules! is somewhat more accessible, but not much.

hlbarber

The problems described here are very real. The solution space is very subtle and the RFC is a great exploration of it. Here's a summary of my thoughts:

Clear wins in this exploration are:

PluginBuilder API.
Upgradable associated type removal.

We should implement these regardless.

The main proposal provides the customer with a super ergonomic solution to both the conditional branching and error message problem. This is given to the customer by default - no intervention is required on their part.

The function signature problem seems more difficult to solve generally. The main proposal prevents the customer from having to work with the O(n) type parameters but the Plugin itself is now required to be known up front. The customer now only has to deal with 1 nasty type parameter when passing a builder through a function signature, but is it still too much of an obstruction? With that said, alternative approach 1 and 2 are strictly less ergonomic and the Either proposal doesn't even attempt to solve it.

Fundamentally, if we want Plugin<P, Op, S, L> to be applied to operations we cannot do it after type erasure of P, Op, S, L. Any solution which preserves the current Plugin architecture and type erases any of these parameters will prevent Plugins being applied afterward. You can see this with the main proposal and the alternative approach 2.

The only feasible solution which solves the conditional branching problem, provides an infallible builder, and prevents eager Plugin is to provide Either<A, B>: Upgradable in my opinion. This does not solve the function signature problem or the error problem and we're still stuck with O(n) type parameters.

In conclusion:

Both lazy type erasure via Box<dyn Upgradable> and Route::new can be used to hold onto the compile time safety of build but are far worse ergonomics for the customer and still require Plugins to be known up front - this probably not a good trade-off.
If we submit to the inevitability of the function signature problem then I think the Either proposal is the primary contestant to the main proposal.

david-perez

What's the plan for solving the same set of problems when building a plugin stack (conditionally applying a plugin and partial initialization of a plugin stack in a function)?

david-perez · 2022-10-24T12:08:10Z

design/src/rfcs/rfc0022_refine_builder.md

+This constraint guarantees that all operation handlers are upgraded to a `Route` using the same set of plugins.
+
+Having to specify all plugins upfront is unlikely to have a negative impact on developers currently using `smithy-rs`.
+We have seen how cumbersome it is to break the startup logic into different functions using the current service builder API. Developers are most likely specifying all plugins and routes in the same function even if the current API allows them to intersperse route registrations and plugin registrations: they would simply have to re-order their registration statements to adopt the API proposed in this RFC.


Developers are most likely specifying all plugins and routes in the same function

This seems to go counter to the Refactoring into smaller functions -> Prepare for some type juggling! section that motivates the proposal for reducing type parameters.

I should have worded this better.
What I meant: since it's so complicated to refactor into smaller functions (given the problems shown in the sections you linked) we can be fairly confident that all the code in the wild is currently sticking to having everything in a single function to avoid the pain (even if they don't necessarily want to).

LukeMathWalker · 2022-11-03T16:47:02Z

Based on our standup discussion from a few days ago:

This RFC does not attempt to solve the same set of problems (branching, refactoring into functions) for building a Plugin stack. We should explore the ergonomics of Plugin stack building in a separate RFC and/or provide dedicated documentation for common patterns that we expect developers to use;
We will move forward with the implementation of this RFC before releasing the new builder API to minimise disruption (e.g. breaking changes) for users.

I'll merge this in and get cracking.

github-actions · 2022-11-03T16:59:01Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
No codegen difference in the Server Test
No codegen difference in the Server Test Python

A new doc preview is ready to view.

RFC draft.

59922c2

LukeMathWalker requested review from a team as code owners October 17, 2022 07:35

Merge branch 'main' into refine-builder

7d6dcfb

LukeMathWalker requested a review from hlbarber October 17, 2022 07:35

Fix checkbox.

c5c0fbd

jjant reviewed Oct 17, 2022

View reviewed changes

hlbarber reviewed Oct 17, 2022

View reviewed changes

david-perez reviewed Oct 17, 2022

View reviewed changes

Briefly describe the strategy in the Overview section.

c9be4e8

Add setter snippet

d8f0126

LukeMathWalker added 3 commits October 17, 2022 17:22

Dive deeper into alternative mechanisms for scoping plugins.

90ae7aa

Add more changes.

a9aab96

Fix snippet.

ac7648e

LukeMathWalker added 4 commits October 18, 2022 16:16

Elaborate on type erasure.

17195c3

Remove dangling footnote.

96b8269

Fix common type.

29f6071

Add more caveats.

0019670

Merge branch 'main' into refine-builder

06a5a88

Update the first approach for lazy type erasure

a66583b

Merge branch 'main' into refine-builder

f560888

hlbarber reviewed Oct 24, 2022

View reviewed changes

hlbarber approved these changes Oct 24, 2022

View reviewed changes

david-perez reviewed Oct 24, 2022

View reviewed changes

david-perez mentioned this pull request Nov 2, 2022

Unhide new service builder and deprecate the prior #1886

Merged

Merge branch 'main' into refine-builder

ca7d5d7

LukeMathWalker enabled auto-merge (squash) November 3, 2022 16:47

LukeMathWalker merged commit 11e9a5f into main Nov 3, 2022

LukeMathWalker deleted the refine-builder branch November 3, 2022 17:18


		### Alternatives: boxed trait objects

		Trait objects are another common approach to achieve type erasure: instead of storing the raw handler in `PokemonServiceBuilder`, we box it and cast it to a `dyn Upgradable<...>`.

[RFC - Server] Refining the service builder API #1859

[RFC - Server] Refining the service builder API #1859

Conversation

LukeMathWalker commented Oct 17, 2022 • edited Loading

Motivation and Context

github-actions bot commented Oct 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hlbarber left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hlbarber Oct 17, 2022 • edited Loading

Choose a reason for hiding this comment

hlbarber Oct 17, 2022 • edited by 82marbag Loading

Choose a reason for hiding this comment

LukeMathWalker Oct 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hlbarber Oct 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-perez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-perez Oct 17, 2022 • edited Loading

Choose a reason for hiding this comment

LukeMathWalker Oct 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 17, 2022

github-actions bot commented Oct 17, 2022

github-actions bot commented Oct 17, 2022

github-actions bot commented Oct 18, 2022

github-actions bot commented Oct 19, 2022

github-actions bot commented Oct 21, 2022

github-actions bot commented Oct 21, 2022

hlbarber Oct 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hlbarber left a comment • edited Loading

Choose a reason for hiding this comment

david-perez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukeMathWalker Oct 24, 2022 • edited Loading

Choose a reason for hiding this comment

LukeMathWalker commented Nov 3, 2022

github-actions bot commented Nov 3, 2022

LukeMathWalker commented Oct 17, 2022 •

edited

Loading

hlbarber left a comment •

edited

Loading

hlbarber Oct 17, 2022 •

edited

Loading

hlbarber Oct 17, 2022 •

edited by 82marbag

Loading

LukeMathWalker Oct 17, 2022 •

edited

Loading

hlbarber Oct 17, 2022 •

edited

Loading

david-perez Oct 17, 2022 •

edited

Loading

LukeMathWalker Oct 17, 2022 •

edited

Loading

hlbarber Oct 24, 2022 •

edited

Loading

hlbarber left a comment •

edited

Loading

LukeMathWalker Oct 24, 2022 •

edited

Loading