Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Api alignment: Config alignment #188

Merged
merged 4 commits into from
Sep 2, 2024

Conversation

DariusIMP
Copy link
Member

Changes:

  • Removing Config.Format enum
  • Immediately returning a Result when creating a config (previously the error would arise only when attempting to open a session).
  • Replacing Config.from(string, format) with Config.fromJson(string), Config.fromJson5(string), Config.fromYaml(string), where each of these functions returns a Result<Config>.
  • Replacing Config.from(file) with Config.fromFile(File file): Result<Config> and Config.fromFile(Path path): Result<Config>.
  • Config.default() still returns a Config (without result, as it's guaranteed it is successfully created by the library).

Internally, when creating a config, the value provided is processed natively, associating each Config instance to a native config instance. The native instance of the config is freed when the Kotlin Config is garbage collected.

…ig, and associating it with a native config.
- renaming Config.from to Config.fromFile
- Adding tests
- Adding or improving KDocs on Config
Copy link

github-actions bot commented Sep 2, 2024

PR missing one of the required labels: breaking-change bug dependencies documentation enhancement new feature internal

@DariusIMP DariusIMP added enhancement New feature or request release Part of the next release labels Sep 2, 2024
///
#[no_mangle]
#[allow(non_snake_case)]
pub extern "C" fn Java_io_zenoh_jni_JNIConfig_00024Companion_loadDefaultConfigViaJNI(
Copy link
Member

Choose a reason for hiding this comment

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

Is this an automatically generated code? The names look a bit strange.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, its not 😅 . It's strange because it's meant to be called from Kotlin through the JNI by a function named loadDefaultConfigViaJNI located at io/zenoh/jni/JNIConfig$Companion. It must have this weird looking signature for Java/Kotlin to locate the corresponding native function.

Copy link
Member

@sashacmc sashacmc left a comment

Choose a reason for hiding this comment

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

LGTM!

@fuzzypixelz fuzzypixelz merged commit 6c7d410 into eclipse-zenoh:main Sep 2, 2024
7 checks passed
@DariusIMP DariusIMP deleted the api/config branch September 2, 2024 16:15
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
enhancement New feature or request release Part of the next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants