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

[FEATURE] Detect name collisions #693

Open
HadrienG2 opened this issue Nov 18, 2022 · 3 comments
Open

[FEATURE] Detect name collisions #693

HadrienG2 opened this issue Nov 18, 2022 · 3 comments

Comments

@HadrienG2
Copy link
Contributor

Is your feature request related to a problem? Please describe.
At this point in time, cursive does not detect when multiple views are given the same name. Puzzling debugging sessions may ensue as call_on_name calls reach the wrong target.

Describe the solution you'd like
Cursive should detect this API usage error and handle it. Panicking would most likely be appropriate here.

Describe alternatives you've considered

  • APIs that name views, such as Nameable::with_name, could return a type that can handle the possibility of error, such as an Option or Result.
  • These APIs could automatically generate derived names e.g. "RequestedName#123" and return that when a requested name is already taken.
  • Usage of user-defined names could be replaced by collision-free alternatives, such as 64-bit integers generated by a global atomic counter.

However, I think this is too much of an edge case to justify expending so much implementation care and/or API surface on it.

@HadrienG2
Copy link
Contributor Author

Digging into the API further, it seems that being able to name multiple views identically is an intended feature. Then perhaps it would be best to allow opting out of this feature through a separate unique identifier mechanism ?

@gyscos
Copy link
Owner

gyscos commented Nov 21, 2022

Hi, and thanks for the report!

I'm not entirely sure how to make the API for this ergonomic.

  • Returning a result when adding a name to a view sounds like a paper cut that will quickly become annoying.
  • Falling back to an alternative available name means users must store the returned value and use that for lookups, preventing the use of literals for that.
  • Most of the time, there are either few unique names, or many names with deterministic (and unique) generated names like "text_{i}".
  • For users that care about this, it is already possible to generate unique identifiers and store that. I could add an example using this, or if it's a common pattern add a method to the Nameable trait to generate (and return) a unique ID.

I suspect a better debugging experience may also help fix issues early in development where the same name is used twice by mistake. Maybe reporting when multiple views exist for a given name?

@HadrienG2
Copy link
Contributor Author

I agree that reporting duplicate names could be sufficient, the tricky part is to find the right communication channel to do so:

  • Logs with <=DEBUG priority are too noisy for the message to stand out, and usually turned off by default anyway for this reason.
  • Logs with >=WARNING priority would probably be inappropriate since using the same name for multiple views is a supported use case.

So maybe an INFO log, or alternatively returning the number of duplicates from with_name/the NamedView constructor could also work (think (Self, usize) tuple). I'm also not terribly convinced by my previous Result & fallback ideas.

A bit of documentation on with_name/NamedView that warns about duplicate names being supported could also be useful.

If you added a unique name generator, something like...

let (named_view, name): (NamedView, Rc<str>) = inner_view.with_unique_name();

...I would personally use that, but it would probably be a good idea to gauge wider interest if you have a way to poll other cursive users.

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

No branches or pull requests

2 participants