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

[red-knot] Consistent suffixes for function names #15569

Closed
sharkdp opened this issue Jan 18, 2025 · 3 comments · Fixed by #15617
Closed

[red-knot] Consistent suffixes for function names #15569

sharkdp opened this issue Jan 18, 2025 · 3 comments · Fixed by #15617
Assignees
Labels
red-knot Multi-file analysis & type inference

Comments

@sharkdp
Copy link
Contributor

sharkdp commented Jan 18, 2025

We previously had a scheme where functions that had a _ty suffix would return a Type<'db>. This has been broken in a few places, for example:

  • bindings_ty which returns a Symbol<'db>
  • declaration_ty which returns a TypeAndQualifiers<'db>

We should probably clean this up, i.e. either come up with new suffixes, with entirely new function names, or decide that we don't want to do anything about it.

@sharkdp sharkdp added the red-knot Multi-file analysis & type inference label Jan 18, 2025
@sharkdp sharkdp self-assigned this Jan 18, 2025
@AlexWaygood
Copy link
Member

AlexWaygood commented Jan 19, 2025

Could we maybe just remove the suffixes? I'm not sure they add much in a Rust codebase, since all functions have return-type annotations anyway.

I also think it's non-obvious to newcomers to the codebase that ty means "type". It took me a while to get used to it and I still don't really love the use of the abbreviation everywhere. (In some cases it's obviously necessary since type is a reserved keyword.)

@MichaReiser
Copy link
Member

I do think the suffixes are useful here or we have to come up with new names entirely. E.g. bindings suggests that it returns an iterator, bindings_ty makes it clear it doesn't.

@sharkdp
Copy link
Contributor Author

sharkdp commented Jan 20, 2025

I opened a #15618 as a proposal on how to rename bindings_ty and declarations_ty, the two functions for which the inconsistency is most apparent.

I also think it's non-obvious to newcomers to the codebase that ty means "type". It took me a while to get used to it and I still don't really love the use of the abbreviation everywhere. (In some cases it's obviously necessary since type is a reserved keyword.)

I opened another draft which renames all *_ty functions to *_type, just to see how it looks like: #15617. This builds on top of #15618. I personally don't have a preference in either direction. If we rename those suffixes from _ty to _type, we should consider doing something similar for variables as well?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
red-knot Multi-file analysis & type inference
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants