- 
                Notifications
    You must be signed in to change notification settings 
- Fork 290
Spin Factors #2519
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
Spin Factors #2519
Conversation
86f4bb7    to
    f1dc5ed      
    Compare
  
    | .component_allowed_hosts | ||
| .get(ctx.app_component().id()) | ||
| .cloned() | ||
| .context("missing component allowed hosts")?; | 
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.
This error message is a bit weak. Perhaps:
| .context("missing component allowed hosts")?; | |
| .with_context(|| format!("missing allowed outbound hosts configuration for component {}", ctx.app_component().id()))?; | 
        
          
                crates/factors/src/prepare.rs
              
                Outdated
          
        
      | } | ||
| } | ||
|  | ||
| pub trait SelfInstanceBuilder: 'static {} | 
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.
This and DefaultInstanceBuilder are nice, but I'm not convinced they're worth it. It seems like they might make things harder to understand in order to save a few lines of boilerplate...
| /// The default implementation returns an empty schema, which accepts any | ||
| /// configuration. | ||
| fn runtime_config_json_schema(&self) -> impl serde::Serialize { | ||
| HashMap::<(), ()>::new() | 
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.
This is somewhat of a placeholder for now, thinking about how to support #2529 (comment)
| // TODO: This seems fine, but then again I don't understand why `FieldOffset`'s | ||
| // own `Sync`ness depends on `U`... | ||
| unsafe impl<T: RuntimeFactors, F1: Factor, F2: Factor> Sync for StateGetter2<T, F1, F2> {} | 
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.
A little spooky, but less spooky than what I had before...
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.
Does this need to stay here? If so, can you try rewriting the comment as to why this is actually safe to do?
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.
I have a fork of field-offset that fixes it, but I'm not sure that anyone is actually maintaining upstream 😬.
If that PR doesn't get merged I'm not sure I even want to keep the dependency...
eeeb140    to
    6fbd5b7      
    Compare
  
    Signed-off-by: Lann Martin <[email protected]>
Signed-off-by: Lann Martin <[email protected]>
Signed-off-by: Lann Martin <[email protected]>
Signed-off-by: Lann Martin <[email protected]>
Signed-off-by: Lann Martin <[email protected]>
Signed-off-by: Lann Martin <[email protected]>
Signed-off-by: Lann Martin <[email protected]>
Signed-off-by: Lann Martin <[email protected]>
Signed-off-by: Lann Martin <[email protected]>
And shorten some generic params. Signed-off-by: Lann Martin <[email protected]>
Signed-off-by: Lann Martin <[email protected]>
Also address a bunch of feedback. Signed-off-by: Lann Martin <[email protected]>
Signed-off-by: Lann Martin <[email protected]>
Signed-off-by: Lann Martin <[email protected]>
Also sneak in Factor::runtime_config_json_schema Signed-off-by: Lann Martin <[email protected]>
Signed-off-by: Lann Martin <[email protected]>
Signed-off-by: Lann Martin <[email protected]>
Signed-off-by: Lann Martin <[email protected]>
Signed-off-by: Lann Martin <[email protected]>
Signed-off-by: Lann Martin <[email protected]>
Signed-off-by: Lann Martin <[email protected]>
Signed-off-by: Lann Martin <[email protected]>
| Closing this in favor of working on the  | 
ref #2518
This has been turned into the
factorsbranch