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

[Docs] Mojo style guide for value parameters #2557

Open
StijnWoestenborghs opened this issue May 6, 2024 · 4 comments
Open

[Docs] Mojo style guide for value parameters #2557

StijnWoestenborghs opened this issue May 6, 2024 · 4 comments
Labels
documentation Improvements or additions to documentation mojo-repo Tag all issues with this label mojo-stdlib Tag for issues related to standard library

Comments

@StijnWoestenborghs
Copy link

StijnWoestenborghs commented May 6, 2024

Where is the problem?

https://github.com/modularml/mojo/blob/4d0b45f74fbcf883c63603a762b0c5c3a11895ed/stdlib/docs/style-guide.md#code-conventions

What can we do better?

Moving a discord discussion to a Github issue here:

I noticed that the style guide (https://github.com/modularml/mojo/blob/4d0b45f74fbcf883c63603a762b0c5c3a11895ed/stdlib/docs/style-guide.md) Is recommending PascalCase for parameters that are values:

fn value parameter    fn repeat[Count: Int]()          PascalCase

(not to be confused with type parameter Action: Actionable which I agree should be PascalCase).
To be honest, I wasn't really doing this for values and it is really confusing with Types and Structs in my opinion.
I also noticed the stdlib isn't following this in most of the cases, and even the docstrings example a couple of lines further of the style guide isn't.

The discussion also included other arguments (some of them listed below):

  • It breaks the nice symmetry between arguments and parameters.
  • If this style is enforced, change an argument to a parameter means renaming everything in the function body
  • Makes function with overload like vectorize look extra bad:
vectorize[func, simd_width, unroll](size)
vectorize[func, simd_width, unroll, size]()
# vs
vectorize[Func, SimdWidth, Unroll](size)
vectorize[Func, SimdWidth, Unroll, Size]()  # why the sudden `Size`?
  • Inconsistent with the guideline on value aliases, as parameters are like local aliases
  • Another problem with using PascalCase for values is that it makes it impossible to use the naming scheme name: Name or size: Size for parameters, which is very common and useful. This is probably the biggest issue tbh, because it will force programmers to spend time coming up with clunkier or more verbose names.

My natural feeling is to use snake_case for these all of these cases. And the stdlib seems to be following this trend. Although the style-guide is merely a recommendation. I would greatly appreciate it to have the community on the same line for this.

@StijnWoestenborghs StijnWoestenborghs added documentation Improvements or additions to documentation mojo-repo Tag all issues with this label labels May 6, 2024
@Benny-Nottonson
Copy link

This is an issue that has proven VERY divisive 👍 But I am on board with whatever the community decides. Personally, I prefer the following syntax.

fn create_tuple[Type: DType, size: Int](value: Float32) -> StaticTuple[Type, size]:

Where Types are in CamelCase, and values are in snake_case, regardless of being an argument or a parameter.

@ematejska ematejska added mojo-repo Tag all issues with this label and removed documentation Improvements or additions to documentation mojo-repo Tag all issues with this label labels May 6, 2024
@nmsmith
Copy link
Contributor

nmsmith commented May 7, 2024

This is an issue that has proven VERY divisive.

So far, everyone who I've seen comment on this topic seems to agree that the casing rules should be uniform across arguments and parameters. So if anything, I'd say that the issue is "unifying", not "divisive". 🙂

@Benny-Nottonson
Copy link

This is an issue that has proven VERY divisive.

So far, everyone who I've seen comment on this topic seems to agree that the casing rules should be uniform across arguments and parameters. So if anything, I'd say that the issue is "unifying", not "divisive". 🙂

That's a lot more along the lines of what I meant, I like your wording a lot more

@ematejska ematejska added the mojo-stdlib Tag for issues related to standard library label May 7, 2024 — with Linear
@JoeLoser JoeLoser added the documentation Improvements or additions to documentation label May 9, 2024
@JoeLoser
Copy link
Collaborator

JoeLoser commented May 9, 2024

@arthurevans can you help us out here with the documentation?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation mojo-repo Tag all issues with this label mojo-stdlib Tag for issues related to standard library
Projects
None yet
Development

No branches or pull requests

5 participants