Generate Dart enum variants in camelCase.

erikas-taroza commented 1 year ago

Changes

Fixes #1110

The generated Dart enums use proper casing. MyEnum.VariantOne -> MyEnum.variantOne

Todo List:

[x] Generate enum with proper variant names.
[x] Handle #[frb(default = "Weekdays.Sunday")]
[x] Handle enum variants that are Dart keywords.

Checklist

[x] An issue to be fixed by this PR is listed above.
[x] New tests are added to ensure new features are working. End-to-end tests are usually in the ./frb_example/pure_dart example, more specifically, rust/src/api.rs and dart/lib/main.dart.
[x] The code generator is run and the code is formatted (via just precommit).
[x] If this PR adds/changes features, documentations (in the ./book folder) are updated.
[x] CI is passing.

Remark for PR creator

Justfile is a task runner for the command line interface that allows you to run shell commands from a file named justfile in your project directory. You can use Justfile after installing it. Note that commands written in justfile of this repository are expected to be run in bash, not cmd or powershell. Running just ... commands in cmd or powershell will produce errors as the syntax is not compatible. On Windows, you can use git bash if you have Git installed.
If fzyzcjy does not reply for a few days, maybe he just did not see it, so please ping him.

welcome[bot] commented 1 year ago

Hi! Thanks for opening this pull request! :smile:

erikas-taroza commented 1 year ago

Hello @fzyzcjy

I have a question about my changes in 28ec345. It addresses the #[frb(default = "Weekdays.Sunday")] macro in api.rs. Since this macro is used in Rust, I think it would make sense to keep the casing in PascalCase for the user. Instead of having them modify the macro, the codegen corrects the casing for Dart. Do the changes in my commit have any side effects? I ran the codegen on the pure_dart example and it seems to work as expected. (I don't know the code base.)

Also, the codegen fails for MyEnum which has variants False and True. These are keywords in Dart when lowercased. How do you think this should be handled? I am thinking about comparing to a list of Dart keywords and deciding if PascalCase is needed. Or the user should not have their variants named after keywords (and let the generated code cause an error).

fzyzcjy commented 1 year ago

Good job!

Do the changes in my commit have any side effects?

Just ensure the CI passes ;) That's one of ways people check whether a code breaks sth unexpectedly on a large codebase

These are keywords in Dart when lowercased. How do you think this should be handled?

Maybe we can make it true_ and false_

fzyzcjy commented 1 year ago

In addition, maybe we should add a flag to the code generator to explicitly enable this feature. Because otherwise, this is a breaking change to existing users.

erikas-taroza commented 1 year ago

Maybe we can make it true and false

The issue is not really with naming but how to detect if the variant in lowercase is a Dart keyword. Maybe a method in IrIdent? Something called is_dart_keyword that would take a string, camelCase it, and be checked in a match statement.

maybe we should add a flag to the code generator to explicitly enable this feature.

Good idea. I'm going to bed so I'll add this when I wake up.

fzyzcjy commented 1 year ago

Sure, take your time!

how to detect if the variant in lowercase is a Dart keyword.

Not looked at your PR code detail yet, but I guess maybe we should do something like, "when you are going to output that (lowercased) variant, check your lowercased variant to see whether it is a keyword"

erikas-taroza commented 1 year ago

Not looked at your PR code detail yet, but I guess maybe we should do something like, "when you are going to output that (lowercased) variant, check your lowercased variant to see whether it is a keyword"

Like a piece of documentation?

In addition, maybe we should add a flag to the code generator to explicitly enable this feature.

I have added a flag called --dart-enums-style.

erikas-taroza commented 1 year ago

In db07d8e, I have added a function in utils.rs to check if the generated variant name is a keyword. If it is a keyword, the casing will remain in PascalCase.

erikas-taroza commented 1 year ago

Not sure why the CI is failing. I updated everything in main.dart :thinking:

frb_example/pure_dart/dart/lib/main.dart:213:32: Error: Member not found: 'friday'.
    expect(r.weekday, Weekdays.friday);
                               ^^^^^^
frb_example/pure_dart/dart/lib/main.dart:374:67: Error: Member not found: 'tuesday'.
    expect(await api.handleReturnEnum(input: "Tuesday"), Weekdays.tuesday);
                                                                  ^^^^^^^
frb_example/pure_dart/dart/lib/main.dart:[379](https://github.com/fzyzcjy/flutter_rust_bridge/actions/runs/4370396073/jobs/7645265735#step:7:380):60: Error: Member not found: 'saturday'.
    expect(await api.handleEnumParameter(weekday: Weekdays.saturday), Weekdays.saturday);
                                                           ^^^^^^^^
frb_example/pure_dart/dart/lib/main.dart:379:80: Error: Member not found: 'saturday'.
    expect(await api.handleEnumParameter(weekday: Weekdays.saturday), Weekdays.saturday);
                                                                               ^^^^^^^^
frb_example/pure_dart/dart/lib/main.dart:399:66: Error: Member not found: 'monday'.
      await api.handleEnumStruct(val: KitchenSink_Enums(Weekdays.monday)),
                                                                 ^^^^^^
frb_example/pure_dart/dart/lib/main.dart:400:34: Error: Member not found: 'tuesday'.
      KitchenSink_Enums(Weekdays.tuesday),
                                 ^^^^^^^
frb_example/pure_dart/dart/lib/main.dart:433:43: Error: Member not found: 'standalone'.
    expect(settings.mode, ApplicationMode.standalone);
                                          ^^^^^^^^^^
frb_example/pure_dart/dart/lib/main.dart:443:39: Error: Member not found: 'embedded'.
                mode: ApplicationMode.embedded,
                                      ^^^^^^^^
frb_example/pure_dart/dart/lib/main.dart:1252:83: Error: Member not found: 'friday'.
  return MyNestedStruct(treeNode: _createMyTreeNode(arrLen: 5), weekday: Weekdays.friday);
                                                                                  ^^^^^^

fzyzcjy commented 1 year ago

Maybe run just precommit in your local computer to see what is wrong. Then, if you want to speed up, see which sub-part in the justfile is going wrong and run that (e.g. just test_whatever')

erikas-taroza commented 1 year ago

Maybe run just precommit in your local computer to see what is wrong. Then, if you want to speed up, see which sub-part in the justfile is going wrong and run that (e.g. just test_whatever')

Nothing changes except for some formatting in Rust.

fzyzcjy commented 1 year ago

Hmm weird. Random guess: can it be some caching problems?

erikas-taroza commented 1 year ago

Hmm weird. Random guess: can it be some caching problems?

Thats what I am thinking as well.

fzyzcjy commented 1 year ago

Btw you may modify the CI yaml file for debugging. For example, cat whatever_file.dart and see its content.

erikas-taroza commented 1 year ago

Btw you may modify the CI yaml file for debugging. For example, cat whatever_file.dart and see its content.

The frb_example/pure_dart/dart/lib/main.dart file is the updated one but the CI is still failing. In Dart test with Valgrind, running the command that caused the error (dart compile exe lib/main.dart -o main.exe) works on my machine as expected.

I have no clue what is happening here.

fzyzcjy commented 1 year ago

main.dart file is the updated

what about the bridge_generated.dart etc

erikas-taroza commented 1 year ago

main.dart file is the updated

what about the bridge_generated.dart etc

:facepalm: Looking at the wrong file. Lets see...

erikas-taroza commented 1 year ago

bridge_definitions.dart contains the enum with the correct names. I'm going to bed now. I will be travelling tomorrow so I may not be able to work on this.

fzyzcjy commented 1 year ago

Take your time and have a nice travelling!

erikas-taroza commented 1 year ago

Changing pure_dart/dart/lib/main.dart to use the old variant names passes the 3 failing tests but fails other tests (like Dart linter) because those are the old variant names.

erikas-taroza commented 1 year ago

Fixed the CI! Just need to let it finish to make sure.

I cloned my fork again and when I opened it in VSCode, it was running codegen. I saw that thebridge_definitions.dart was generated using the Rust style variant names. It turns out that the build.rs in the pure_dart example was running the codegen without the new flag and thus causing the error. Now it makes sense why those 3 tests were failing. They were running cargo build.

What a sneaky bug!

fzyzcjy commented 1 year ago

Good job! I get a bit confused: Why just precommit does not figure out this bug? Then it seems like a bug in infra as well - just precommit should at least run codegen (implicitly) using build.rs so this should be spotted at the beginning.

erikas-taroza commented 1 year ago

Good job! I get a bit confused: Why just precommit does not figure out this bug? Then it seems like a bug in infra as well - just precommit should at least run codegen (implicitly) using build.rs so this should be spotted at the beginning.

I'm on Fedora so maybe I missed a command? I have to copy and paste the commands, it doesn't work OOB. I am too lazy to configure it.

fzyzcjy commented 1 year ago

I see. Will review this PR in probably today :)

erikas-taroza commented 1 year ago

I see. Will review this PR in probably today :)

Sounds good :)

erikas-taroza commented 1 year ago

@fzyzcjy Fixed :)

welcome[bot] commented 1 year ago

Hi! Congrats on merging your first pull request! :tada:

fzyzcjy commented 1 year ago

@all-contributors please add @erikas-taroza for code

allcontributors[bot] commented 1 year ago

@fzyzcjy

I've put up a pull request to add @erikas-taroza! :tada:

fzyzcjy / flutter_rust_bridge