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

Migrate most tool commands to dart #321

Merged
merged 10 commits into from
Jan 19, 2022

Conversation

HakkyuKim
Copy link
Contributor

@HakkyuKim HakkyuKim commented Jan 5, 2022

Migrate all commands except integration test to dart. Some commands are replaced to similar ones in flutter_plugin_tools to reduce code maintenance, in turn there are some notable changes which are summarized below:

  • command option changes:
    • option name change, plugins -> packages: Due to the implementation of PluginCommand in flutter_plugin_tools. This change doesn't affect integration-test at the moment because it's not migrated to dart yet.
    • --run-on-changed-packages: Due to the implementation of PluginCommand in flutter_plugin_tools, if no packages have changed, the option makes command run on all packages. This makes sense since most changes outside packages are CI scripts and tool changes which may affect all packages. This change doesn't affect integration-test at the moment because it's not migrated to dart yet.
    • option --run-on-changed-packages can't be used together with option packages(previously plugins): This will make implementating integration-test command in dart with the current --recipe logic(Update the test recipe and workflow #293 (comment)) difficult so the behavior of recipe may change.
  • subcommand name changes:
    • build -> build-examples (to be consistent with flutter_plugin_tools)
    • plugins -> list (using command ListCommand from flutter_plugin_tools)
    • test -> integration-test (to differentiate from unit test which may be added in the future)
    • tidy -> format (using command FormatCommand from flutter_plugin tools)
  • subcommand behavior changes:
    • format (previously tidy): formats dart, java, objective-c, and c++. Internally it uses clang-format --style=Google, @bbrto21 any reasons for specifically using clang-format-11 with --style=file? clang-format(clang-format-10 in ubuntu 20.04) and clang-format-11 generate the same rules for --style=Google. For dart, it runs flutter format which is a wrapper of dart format.

@github-actions github-actions bot added the tools label Jan 5, 2022
@HakkyuKim HakkyuKim marked this pull request as draft January 5, 2022 01:46
@HakkyuKim HakkyuKim force-pushed the migrate-commands-in-dart branch from 17931d7 to 2f8daed Compare January 5, 2022 02:44
@HakkyuKim
Copy link
Contributor Author

Test will pass after #322.

@HakkyuKim HakkyuKim force-pushed the migrate-commands-in-dart branch from 2f8daed to 55c9e4e Compare January 6, 2022 00:44
@swift-kim
Copy link
Member

swift-kim commented Jan 6, 2022

any reasons for specifically using clang-format-11 with --style=file? clang-format(clang-format-10 in ubuntu 20.04) and clang-format-11 generate the same rules for --style=Google.

As far as I remember, clang-format-11 and clang-format-12 had different formatting rules when we tested before. I'm not sure about the clang-format-10 case. We just wanted to make sure every user uses the same clang-format version. (The version is also specified in https://github.com/flutter-tizen/flutter-tizen/wiki/Style-guide for this reason.) You can change the version if you need.

@HakkyuKim HakkyuKim marked this pull request as ready for review January 6, 2022 01:26
@HakkyuKim
Copy link
Contributor Author

Ah, you're right. clang-format 10, 11, and 12 all dump slightly different config files for --style=Google. Our plugin C++ code just didn't have cases where those different rules would apply. I'll actually change back to using clang-format-11 as it's possible to provide a custom clang-format executable in FormatCommand, it will also prevent unnecessary confusion about which version we're using.

tools/lib/src/build_examples_command.dart Outdated Show resolved Hide resolved
tools/lib/src/build_examples_command.dart Outdated Show resolved Hide resolved
tools/lib/src/integration_test_command.dart Show resolved Hide resolved
tools/lib/src/integration_test_command.dart Outdated Show resolved Hide resolved
tools/lib/src/integration_test_command.dart Outdated Show resolved Hide resolved
tools/lib/src/integration_test_command.dart Outdated Show resolved Hide resolved
tools/lib/src/integration_test_command.dart Outdated Show resolved Hide resolved
tools/lib/src/integration_test_command.dart Outdated Show resolved Hide resolved
tools/lib/src/integration_test_command.dart Outdated Show resolved Hide resolved
tools/lib/src/main.dart Outdated Show resolved Hide resolved
@HakkyuKim HakkyuKim force-pushed the migrate-commands-in-dart branch from fd62923 to 20eba93 Compare January 14, 2022 05:13
@HakkyuKim HakkyuKim merged commit ab57fb8 into flutter-tizen:master Jan 19, 2022
bwikbs pushed a commit to bwikbs/plugins that referenced this pull request Mar 26, 2022
* Migrate most commands to dart

* Update github workflow scripts

* Register and parse arguments for integration test

* Code cleanups and remove .clang-format file

* Use clang-format-11

* Restore .clang-format file

* Fix comments and log messages

* Format code and log messages

* Remove newlines in help messages

* Fix help message
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants