Skip to content
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

Unify constructors accepting names #2071

Closed
addisoncrump opened this issue Apr 18, 2024 · 8 comments
Closed

Unify constructors accepting names #2071

addisoncrump opened this issue Apr 18, 2024 · 8 comments
Labels
enhancement New feature or request

Comments

@addisoncrump
Copy link
Collaborator

addisoncrump commented Apr 18, 2024

To be applied after #2065

We currently use each of the following in constructors for Named types:

  • &'static str
  • String
  • Cow<'static, str>
  • S with where S: Into<String>
  • S with where S: Into<Cow<'static, str>>

We should unify this to a common interface. My vote is personally for the first, since we can enforce that people statically name their observers (which is generally preferable). If there are legitimate reasons we can think of that people might not want to, then we should go with the last (which uses the new Copy-on-Write pattern we're introducing in #2065).

@addisoncrump
Copy link
Collaborator Author

cc @tokatoka @domenukk, let's decide on this and then open it up to newcomers after #2065 lands.

@addisoncrump addisoncrump added the enhancement New feature or request label Apr 18, 2024
@tokatoka
Copy link
Member

First is good to me

@addisoncrump
Copy link
Collaborator Author

There is also actually another option here:

  • S with where S: AsRef<str>

This might be the most idiomatic if we allow for non-static names.

@domenukk
Copy link
Member

I don't think &'static str will work for dynamic types (?)

@addisoncrump
Copy link
Collaborator Author

Can you specify what scenario you're thinking of?

@domenukk
Copy link
Member

I mean just in general? Like, this guy?

name: format!("differential_{}_{}", first.name(), second.name()),

@addisoncrump
Copy link
Collaborator Author

Right, that's how it's stored internally, not how we accept it as a parameter 😅

@domenukk
Copy link
Member

But users may want to pass in names dynamically (maybe?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants