Skip to content

Refactor type - #1591

Merged
HosseinYousefi merged 9 commits into
mainfrom
refactor-type
Sep 24, 2024
Merged

Refactor type#1591
HosseinYousefi merged 9 commits into
mainfrom
refactor-type

Conversation

@HosseinYousefi

@HosseinYousefi HosseinYousefi commented Sep 23, 2024

Copy link
Copy Markdown
Contributor

Addresses parts of #1581
Closes #664
Addresses parts of #614

  • Made $type, reference, fromReference, inner type params and ... internal
  • Made everything in the type classes internal, they are still needed by the other bindings
    • Users now must use Foo.type and GenericFoo.type(...) to create the type classes instead of $Foo$Type(...).
    • I could alternatively make the type classes internal altogether, but that messes with exporting bindings in a package as users would have to hide them one by one. Alternatively we could generate .t.dart files that only contain the typeclasses but that seemed to be overkill for what this realistically does, which is making type class creation syntax consistent.
  • Renamed castTo to as to be closer to Dart casting syntax.
  • Relaxed renaming keywords, some identifiers are not possible to use as class names but they are possible to use as method and field names.
  • Renamed the type class names from $FooType to $Foo$Type to prevent name collisions.

@github-actions

github-actions Bot commented Sep 23, 2024 •
edited
Loading

Copy link
Copy Markdown

PR Health

Coverage ⚠️
File Coverage
pkgs/jni/example/lib/main.dart 💔 Not covered
pkgs/jni/lib/_internal.dart 💔 Not covered
pkgs/jni/lib/jni.dart 💔 Not covered
pkgs/jni/lib/src/accessors.dart 💔 Not covered
pkgs/jni/lib/src/jarray.dart 💔 Not covered
pkgs/jni/lib/src/jimplementer.dart 💔 Not covered
pkgs/jni/lib/src/jni.dart 💔 Not covered
pkgs/jni/lib/src/jobject.dart 💔 Not covered
pkgs/jni/lib/src/jprimitives.dart 💔 Not covered
pkgs/jni/lib/src/lang/jboolean.dart 💔 Not covered
pkgs/jni/lib/src/lang/jbyte.dart 💔 Not covered
pkgs/jni/lib/src/lang/jcharacter.dart 💔 Not covered
pkgs/jni/lib/src/lang/jdouble.dart 💔 Not covered
pkgs/jni/lib/src/lang/jfloat.dart 💔 Not covered
pkgs/jni/lib/src/lang/jinteger.dart 💔 Not covered
pkgs/jni/lib/src/lang/jlong.dart 💔 Not covered
pkgs/jni/lib/src/lang/jnumber.dart 💔 Not covered
pkgs/jni/lib/src/lang/jshort.dart 💔 Not covered
pkgs/jni/lib/src/lang/jstring.dart 💔 Not covered
pkgs/jni/lib/src/method_invocation.dart 💔 Not covered
pkgs/jni/lib/src/nio/jbuffer.dart 💔 Not covered
pkgs/jni/lib/src/nio/jbyte_buffer.dart 💔 Not covered
pkgs/jni/lib/src/types.dart 💔 Not covered
pkgs/jni/lib/src/util/jiterator.dart 💔 Not covered
pkgs/jni/lib/src/util/jlist.dart 💔 Not covered
pkgs/jni/lib/src/util/jmap.dart 💔 Not covered
pkgs/jni/lib/src/util/jset.dart 💔 Not covered
pkgs/jnigen/example/in_app_java/lib/android_utils.dart 💔 Not covered
pkgs/jnigen/example/in_app_java/lib/main.dart 💔 Not covered
pkgs/jnigen/example/kotlin_plugin/lib/kotlin_bindings.dart 💔 Not covered
pkgs/jnigen/example/notification_plugin/lib/notifications.dart 💔 Not covered
pkgs/jnigen/example/pdfbox_plugin/example/lib/main.dart 💔 Not covered
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/pdmodel/PDDocument.dart 💔 Not covered
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/pdmodel/PDDocumentInformation.dart 💔 Not covered
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/text/PDFTextStripper.dart 💔 Not covered
pkgs/jnigen/lib/src/bindings/dart_generator.dart 💔 Not covered
pkgs/jnigen/lib/src/bindings/renamer.dart 💔 Not covered
pkgs/jnigen/lib/src/bindings/resolver.dart 💔 Not covered
pkgs/jnigen/lib/src/elements/elements.dart 💔 Not covered

This check for test coverage is informational (issues shown here will not fail the PR).

This check can be disabled by tagging the PR with skip-coverage-check.

License Headers ⚠️ 
// Copyright (c) 2024, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.
Files
pkgs/jnigen/example/in_app_java/lib/android_utils.dart
pkgs/jnigen/example/kotlin_plugin/lib/kotlin_bindings.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/pdmodel/PDDocument.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/pdmodel/PDDocumentInformation.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/text/PDFTextStripper.dart

All source files should start with a license header.

Unrelated files missing license headers
Files
pkgs/ffigen/example/libclang-example/generated_bindings.dart
pkgs/ffigen/example/shared_bindings/generate.dart
pkgs/ffigen/example/shared_bindings/lib/generated/a_gen.dart
pkgs/ffigen/example/shared_bindings/lib/generated/a_shared_b_gen.dart
pkgs/ffigen/example/shared_bindings/lib/generated/base_gen.dart
pkgs/ffigen/example/simple/generated_bindings.dart
pkgs/ffigen/lib/src/header_parser/clang_bindings/clang_bindings.dart
pkgs/ffigen/test/collision_tests/expected_bindings/_expected_decl_decl_collision_bindings.dart
pkgs/ffigen/test/collision_tests/expected_bindings/_expected_decl_symbol_address_collision_bindings.dart
pkgs/ffigen/test/collision_tests/expected_bindings/_expected_decl_type_name_collision_bindings.dart
pkgs/ffigen/test/collision_tests/expected_bindings/_expected_reserved_keyword_collision_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_comment_markup_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_dart_handle_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_enum_int_mimic_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_forward_decl_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_functions_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_imported_types_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_native_func_typedef_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_opaque_dependencies_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_packed_structs_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_regress_384_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_struct_fptr_fields_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_typedef_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_unions_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_varargs_bindings.dart
pkgs/ffigen/test/large_integration_tests/_expected_cjson_bindings.dart
pkgs/ffigen/test/large_integration_tests/_expected_libclang_bindings.dart
pkgs/ffigen/test/large_integration_tests/_expected_sqlite_bindings.dart
pkgs/ffigen/test/native_test/_expected_native_test_bindings.dart
pkgs/jni/lib/src/third_party/generated_bindings.dart
pkgs/jni/lib/src/third_party/global_env_extensions.dart
pkgs/jni/lib/src/third_party/jni_bindings_generated.dart
pkgs/jnigen/android_test_runner/lib/main.dart
pkgs/jnigen/example/kotlin_plugin/example/lib/main.dart
pkgs/jnigen/example/kotlin_plugin/lib/kotlin_plugin.dart
pkgs/jnigen/example/pdfbox_plugin/lib/pdfbox_plugin.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/pdmodel/_package.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/text/_package.dart
pkgs/jnigen/lib/src/bindings/descriptor.dart
pkgs/jnigen/lib/src/elements/elements.g.dart
pkgs/jnigen/test/jackson_core_test/third_party/bindings/com/fasterxml/jackson/core/_package.dart
pkgs/jnigen/tool/command_runner.dart
pkgs/native_assets_builder/test_data/native_dynamic_linking/bin/native_dynamic_linking.dart
pkgs/swift2objc/lib/src/config.dart
pkgs/swift2objc/lib/src/generate_wrapper.dart
pkgs/swift2objc/lib/src/generator/_core/utils.dart
pkgs/swift2objc/lib/src/generator/generator.dart
pkgs/swift2objc/lib/src/generator/generators/class_generator.dart
pkgs/swift2objc/lib/src/parser/parsers/declaration_parsers/parse_initializer_declaration.dart
pkgs/swift2objc/lib/src/parser/parsers/declaration_parsers/parse_property_declaration.dart
pkgs/swift2objc/lib/src/transformer/_core/unique_namer.dart
pkgs/swift2objc/lib/src/transformer/_core/utils.dart
pkgs/swift2objc/lib/src/transformer/transformers/transform_property.dart
pkgs/swift2objc/test/unit/parse_initializer_param_test.dart

This check can be disabled by tagging the PR with skip-license-check.

Package publish validation ✔️
Package Version Status
package:ffi 2.1.3 already published at pub.dev
package:ffigen 15.0.0-wip WIP (no publish necessary)
package:jni 0.12.0-wip WIP (no publish necessary)
package:jnigen 0.12.0-wip WIP (no publish necessary)
package:native_assets_cli 0.8.1-wip WIP (no publish necessary)
package:objective_c 2.1.0-wip WIP (no publish necessary)
package:swift2objc 0.0.1-wip WIP (no publish necessary)
package:swiftgen 0.0.1-wip WIP (no publish necessary)

Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation.

@coveralls

coveralls commented Sep 24, 2024 •
edited
Loading

Copy link
Copy Markdown

Coverage Status

coverage: 90.886% (-0.2%) from 91.042%
when pulling ed3addf on refactor-type
into 82c0aac on main.

dcharkes
dcharkes approved these changes Sep 24, 2024
View reviewed changes

@dcharkes dcharkes left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Closes #664
Addresses parts of #614

I'd have expected some tests exercising those use cases. Have a test with a method name that would have been shadowed by the previous naming scheme. And have a test that relies on some keyword not be renamed in some context.

LGTM!

Comment thread pkgs/jni/test/type_test.dart Outdated
@override
String get signature => 'B';

@internal

@dcharkes dcharkes Sep 24, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

duplicate annotations?

@HosseinYousefi HosseinYousefi Sep 24, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/@internal\n\s+@internal/@internal/!

@HosseinYousefi

HosseinYousefi commented Sep 24, 2024

Copy link
Copy Markdown
Contributor Author

I'd have expected some tests exercising those use cases. Have a test with a method name that would have been shadowed by the previous naming scheme. And have a test that relies on some keyword not be renamed in some context.

Will be addressed by #569

@HosseinYousefi
HosseinYousefi merged commit 117cc75 into main Sep 24, 2024
@HosseinYousefi
HosseinYousefi deleted the refactor-type branch September 24, 2024 06:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dcharkes dcharkes dcharkes approved these changes
@liamappelbe liamappelbe Awaiting requested review from liamappelbe

Assignees

No one assigned

Labels

package:jni package:jnigen skip-changelog-check

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Keywords are too restrictive

3 participants

@HosseinYousefi @coveralls @dcharkes