[native_assets_cli] Make CCompilerConfig fields less nullable
#1809
Conversation
| expect(codeConfig.cCompiler.compiler, fakeClang); | ||
| expect(codeConfig.cCompiler.linker, fakeLd); | ||
| expect(codeConfig.cCompiler.archiver, fakeAr); | ||
| expect(codeConfig.cCompiler?.compiler, fakeClang); |
There was a problem hiding this comment.
nit: Should this use ! - as we expect it to be non-nullable?
There was a problem hiding this comment.
I think the guidance is to prefer writing your tests in a way so that the error message is the most useful on failure:
https://pub.dev/packages/matcher
Prefer semantically meaningful matchers to comparing derived values.
Matchers which have knowledge of the semantics that are tested are able to emit more meaningful messages which don't require reading test source to understand why the test failed.
A stack trace showing that ! failed will give no error message about the fakeClang path that was expected.
| @@ -10,13 +10,13 @@ import '../utils/map.dart'; | |||
| /// The configuration for a C toolchain. | |||
| final class CCompilerConfig { | |||
| /// Path to a C compiler. | |||
| late final Uri? compiler; | |||
| late final Uri compiler; | |||
There was a problem hiding this comment.
@dcharkes you recently brought up the question why I named it config.codeConfig and the class CodeConfig and we said we could drop "config" iff we structured the LinkConfig differently.
=> The same applies here, would we want to rename CCompilerConfig -> CCompiler?
There was a problem hiding this comment.
I think the class name should still include config. But the fields not. I have a CL that does that, but I'm not fully happy with how it looks, maybe it feels more natural to do config.codeConfig.androidConfig.ndkVersion rather than config.code.android.ndkVersion, I'll play around with it and send it out as draft PR for us to decide what we want.
Addressing:
BuildConfig.targetOStoBuildConfig.codeConfig.targetOS, remove it entirely or extend to cover web #1738 (comment)Currently on Flutter and the Dart CI provide this information, and they always provide all 3. So this should not be a breaking change on the protocol level.
It is a breaking change on the API level.