feat: add Methods::merge_replace

[mattsse]

Closes #1053

add merge_replace that replaces callbacks

[niklasad1]

So this is bit tricky to use I reckon now as it's easy to override something and not realize it.

As I see it we could have merge_by as well

enum Insert {
 Yes,
 No,
}

fn merge_by<F: Fn(&str, &str) -> Insert)(&mut self, other: impl Into<Methods>, f: F) { ...  }

// example
methods.merge_by(other, |existing, other| {
   // method names are equal, just throw a warning log if that occurs
   // this may be expected, up to user to decide what's an issue or not
   if existing == other {
      tracing::warn!("Overriding method: `{method}`");
   }
   Insert::Yes
}); 

[niklasad1]

/cc @lexnv @jsdw what do you guys think?

[jsdw]

I could get behind that (I might be tempted to call it .merge_if and have it return a bool, true to merge).

If that existed, could merge be then a simple wrapper around it?
Would this function also need the self.verify_method_name(name)?; check that merge has?

[niklasad1]

Would this function also need the self.verify_method_name(name)?; check that merge has?

no, that only checks if a method with the same name exist and this API leaves that to the user to decide i.e, if two methods with the same name occurs then either replace it with the new one or something else.

With "our existing merge API" the entire RPC module is thrown away if that occurs

[lexnv]

If we had |existing: &str, other: &str|, what would be the value of existing if the method did not prior to merging exist?

Another idea would be to have the callback fire on collisions only:

fn merge_if<F: Fn(&str) -> bool)(&mut self, other: impl Into<Methods>, f: F) { ...  }

methods.merge_if(other, |collision_name|| {
   if collision_name == "CoreFn" {
       warn!("Don't want to replace `{collision_name}`;
       return false;
   }
   true

Indeed we should fire the callback only if:

• Entry::Occupied(..) similar to verify_method_name
• name collides

I feel that both solutions limit the user to pick either the existing or the new method.
For that contributor's use case, the function should always be |_collision_name| { true }.

As a crazy thought, we could probably chain methods, but Id have a hard time arguing the complexity of the code is worth this kind of runtime sorcery:

fn map<F: Fn(&str, &mut MethodCallback )(&mut self, f: F) { ...  }

methods.map(other, |(existing_name, existing_method)| {
   if existing_name == "CoreFn" {
       // Modify existing_method: 
       // Chain it with some other callback, or replace it directly
       info!("Modifying `{existing_name}`;
   }

[mattsse]

So this is bit tricky to use I reckon now as it's easy to override something and not realize it.

I guess this is true, even if this is to be used as the last "merge" call, this could possibly cause problems.

I think merge_if with a callback that decides whether to replace an existing callback or not makes this explicit.

[niklasad1]

The CI is borked, not sure if it's the latest tokio release that caused that but not related to this PR