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

Name casing in generated code #280

Open
sveinnthorarins opened this issue Oct 13, 2023 · 5 comments
Open

Name casing in generated code #280

sveinnthorarins opened this issue Oct 13, 2023 · 5 comments
Labels
enhancement New feature or request

Comments

@sveinnthorarins
Copy link

Is your feature request related to a problem? Please describe.

Release v2.8.0 fixed a bug where name casing was not applied to generated code.
So after updating, the names of all the values for my enum type are now different when generated.

I'll show you a minimal example of what I'm working with...

The enum is called MessageType and is supposed to denote of which type each message is. It has a lot of values so I decided to prefix values with 's' (server) or 'c' (client) to distinguish which actor sends this type of message. So as a result, the values are named with camel case in my message.bop file. The camel case name casing used to carry over to the generated code but now the values are named with pascal case in the generated code.

// message.bop
enum MessageType: uint8 {
    sUserJoined = 10;
    sUserLeft = 11;
    cChooseUsername = 20;
}

// message.ts before v2.8.0
export enum MessageType {
    sUserJoined = 10;
    sUserLeft = 11;
    cChooseUsername = 20;
}

// message.ts after v2.8.0
export enum MessageType {
    SUserJoined = 10;
    SUserLeft = 11;
    CChooseUsername = 20;
}

This causes two different choices of frustration points that I can go for, either I have to always change the generated file after generation or I have to change every line in my codebase that uses the generated enum type and accept this uglier (in my opinion) type of name casing.

Describe the solution you'd like

The first solution that came to mind was to check if there were any arguments for name casing I could give the compiler, but I've looked around in the documentation and can't find a list of possible arguments anywhere (documentation seems to mainly show how to install and lacks documentation of how to actually use tools).

If there is such a list anywhere in the docs and/or such an argument exists, let me know... Otherwise I'd like to propose such an argument option. The options could be for example to make compiler (1) change to pascal case, (2) change to camel case, (3) no case changes, i.e. use the casing as it is in the .bop file.

Let me know what you think. Oh, and thanks for creating and maintaining this project, it rules! :)

@sveinnthorarins sveinnthorarins added the enhancement New feature or request label Oct 13, 2023
@sveinnthorarins sveinnthorarins changed the title Name casing Name casing in generated code Oct 13, 2023
@andrewmd5
Copy link
Contributor

Thanks for filing this report. I can see why this is causing some frustration - I should say I’m not opposed to adding a flag that preserves casing, however, in general ambiguous suffixes collide with best practices (not that we’ve outlined them in our documentation anywhere).

I assume you have one definition that you use MessageType as a field in hence combining server and client types into a single enum. Have you considered utilizing a union? It might look something like:

enum ClientMessageType: uint8 {
    ChooseUsername = 20;
}
enum ServerMessageType : uint8 {
  UserJoined = 10;
  UserLeft = 11;
}

union NetworkMessage {
    1 -> message ClientMessage {
        1 -> ClientMessageType type;
    } 
    2 -> message ServerMessage {
        1 -> ServerMessageType type;
    } 
}

and then in you’re code you’re able to do something like:

//type guard
if (decoded.IsServerMessage) {
   console.log(decoded.value.type)
}

@sveinnthorarins
Copy link
Author

sveinnthorarins commented Oct 14, 2023

Well, I haven't looked much at unions but the distinction between actors is not necessarily that important. It just felt nice being able to quickly discern the actor when using the generated enum value constants in code. I also have some both-actor values (like "Error") which would require the third enum type so I think I'd prefer not complicating things with separate enums and a union. Also, I am using a struct, not a message, in an effort to try to take up the least bits possible. Not sure if that complicates using a union even further.

Anyways, the bebop compiler changing casing just felt like it was a little bit too opinionated. Now, to be fair, I felt extra frustrated because it didn't change the casing when I wrote the .bop file. If it did I wouldn't have gone with the "s" and "c" prefixes and maybe would have opted for no prefixes or just "Server" and "Client" prefixes instead. I don't know how I would have felt but I probably wouldn't have been frustrated enough over the compiler's opinions to raise an issue 😄 So yeah, most people probably don't have a problem with the compiler changing casing.

But nevertheless, I still think it is worthwhile to re-evaluate whether the compiler should be forcing a certain name casing opinion and whether the developer should be able to control this behavior. I would fully understand changing the casing if it was necessary to support a certain programming language where casing is a language feature, for example a visibility modifier like in Go. However, if changing the casing isn't strictly necessary, it feels to me like preserving casing and respecting the .bop file would be a better design that allows for more versatility and easier incorporation of bebop types in codebases. At this point thought it is probably better to make the smallest change possible, i.e. adding a flag that preserves casing would be sufficient to allow people to adapt bebop to their needs.

@andrewmd5
Copy link
Contributor

Not sure if that complicates using a union even further.

Unions support messages and structs intermingled.

Anyways, the bebop compiler changing casing just felt like it was a little bit too opinionated.

The goal of the compiler is to generate code that conforms with the conventions of the target language; though TypeScript documentation doesn't make a specific recommendation of enum member naming, they exclusively use StrictPascalCase and UPPER_CASE (both of which are reasonable). We opt for StrictPascalCase which requires making the first character uppercase, and in your case that leads to the oddly formatted members.

The change was more-so a bug fix because we weren’t doing that conversion in a few places in the generator and a corner case caught badly generated code as a result.

Bebop also recommends using StrictPascalCase for enum members because it converts to other cases based on that when targeting other languages.

All that being said, converting of casing to match coding conventions of a given language is a feature, and I’m hesitant to add a new flag that disables it (if I did it would be hidden from —help); I would recommend refactoring your schema to fully spell out Client/Server or restructure to better identify logical bits of data, as it will be a week or so before I can reach a decision.

I appreciate you taking time to explain everything and for being a user

@sveinnthorarins
Copy link
Author

Okay, yeah I will refactor my schema as that's the easiest option right now.

The feature of converting casing to match coding conventions of the targeted language definitely makes sense. It would be handy to have the docs mention this somewhere though. And, I also couldn't see the docs recommending StrictPascalCase for enum members, would be awesome to add that too.

Thanks Andrew for taking my issue into consideration 😀

@MrFoxPro
Copy link

MrFoxPro commented Apr 15, 2024

I would like to preserve casing for typescript as well

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