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

Code action: Convert variable names to snake_case #2978

Open
jfmengels opened this issue Apr 12, 2024 · 9 comments · May be fixed by #3273
Open

Code action: Convert variable names to snake_case #2978

jfmengels opened this issue Apr 12, 2024 · 9 comments · May be fixed by #3273
Labels
good first issue Good for newcomers help wanted Contributions encouraged priority:medium

Comments

@jfmengels
Copy link

jfmengels commented Apr 12, 2024

Coming from other languages like Elm and JavaScript where the conventions are to have variables named in camel-case.

In Gleam, camel-case is forbidden and results in syntax errors, which I can very much understand as it should make for very consistent naming patterns.
My force of habit is strong, and I spend quite some time converting the camel-case names I write to snake-case, which is distracting from my goal at hand.

A suggestion I have that could make that easier, is have the formatter automatically convert names to snake-case.

For instance, the formatter could change the code like this

fn doSomething() { todo }
-->
fn do_something() { todo }

The compiler error already suggests this name change in the error message.

Same idea for references to consts/functions in expressions.

If this can be done reliably, then I can't think of many downsides (but I may not be familiar enough with the syntax).

One downside I can imagine is that if you've never written Gleam, and you don't ever see the error message that variables should be snake_case, then it will feel quite jarring to see a rename. Once you know it, I think it will be helpful though.

@jfmengels
Copy link
Author

jfmengels commented Apr 13, 2024

Alternatively, this could be an LSP code action. Requires more manual work from the user but it would be less surprising.

Some editors sometimes have a "Convert to snake_case" command which solves the problem, but it's not necessarily discoverable. Took me a few days to discover mine had that. An LSP code action would be a lot more discoverable.

@lpil
Copy link
Member

lpil commented Apr 17, 2024

Sounds good to me! I think we would only want to do this for definitions and not for referencing variables and functions. What do you think?

We'll need to make the parser permit uppercase names have the analyser emit an error if one is used in a definition.

@lpil lpil added help wanted Contributions encouraged discussion The approach has not yet been decided good first issue Good for newcomers priority:medium labels Apr 17, 2024
@jfmengels
Copy link
Author

I could see it used for both definitions and references, but I do imagine this being more surprising or annoying for references. I think it would be reasonable to start only with definitions, and at some point to have a branch where it also does it for references, and try to see how it feels.

Although it should probably be tried out by beginners, as seasoned Gleam developers will likely not write camelcase already (unless they also often work with camelcase languages).

@lpil
Copy link
Member

lpil commented Apr 19, 2024

For references it could "correct" to a value that type checks but isn't the value that the programmer intended to use. I'd like to not have anything that could make the programmer's code silently do the wrong thing.

@GearsDatapacks
Copy link
Contributor

GearsDatapacks commented Jun 11, 2024

I might try and tackle this one. Should I implement it in the formatter or in as an LSP action?

And should I take a look at what was suggested in #2998, and change this error to occur in the analyser?

@GearsDatapacks
Copy link
Contributor

Thinking about this more, I think this should be an lsp action, since the job of the formatter isn't really to fix errors in code.

@lpil
Copy link
Member

lpil commented Jun 12, 2024

Yes I think so too!

@lpil lpil changed the title Formatter: Convert variable names to snake-case Code action: Convert variable names to snake-case Jun 12, 2024
@lpil lpil changed the title Code action: Convert variable names to snake-case Code action: Convert variable names to snake_case Jun 12, 2024
@lpil lpil removed the discussion The approach has not yet been decided label Jun 12, 2024
@GearsDatapacks
Copy link
Contributor

One question about the implementation of this: Should I unify the three name token types and only discriminate between them later, or keep them and just not report any errors until the analyser?

@lpil
Copy link
Member

lpil commented Jun 12, 2024

Keep them distinct please. The change to the codebase should be small.

@GearsDatapacks GearsDatapacks linked a pull request Jun 13, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Contributions encouraged priority:medium
Projects
Status: Unfinished
Development

Successfully merging a pull request may close this issue.

3 participants