feat(maven): Optional android config - #271
Conversation
|
it prints |
| this.logger.warn( | ||
| 'You may need the Android configuration and it was not found in the configuration file.' | ||
| ); |
There was a problem hiding this comment.
not sure if this should be a warn level since the android config is optional.
There was a problem hiding this comment.
I find anything below a warn to not be enough when an android release may file because the config is missing.
There was a problem hiding this comment.
Then this should be info or even less and then we should have a hard-fail when this is required but missing.
|
@iker-barriocanal @BYK there are a few comments here but the PR does whats supposed to do, feel free to address them, or not and merge and release Craft, I can confirm that it works :) |
btw it already prints |
android configandroid config
This patch requires an Android config. Should no android config be provided, set `android: false`
BYK
left a comment
There was a problem hiding this comment.
Definitely an improvement over the existing state so approving. That said I have some questions, concerns, and suggestions.
| mavenSettingsPath: DEFAULT_OPTION_VALUE, | ||
| mavenRepoId: DEFAULT_OPTION_VALUE, | ||
| mavenRepoUrl: DEFAULT_OPTION_VALUE, | ||
| android: false, |
There was a problem hiding this comment.
Why not null or undefined? I'm wary about variable-type fields.
There was a problem hiding this comment.
My thinking about what this config field should answer is "I don't want to configure Android".
- I think of
nullas "there's no Android config". Why isn't there an Android config? Have you forgotten about it? This, to me, is like not answering a question. - I think of
undefinedas "the Android config hasn't been defined". Why don't you define it then, if it's a config file? Do you want to set anandroidconfig or not? This, to me, is like giving a very ambiguous answer to a question. - I think of
falseas "I don't want to set an Android config, but I'm saying it". This, to me, is like saying you don't want to set the configuration (answering with a nonambiguous answer).
Thus, using false seems the most appropriate for me. Do you have a different opinion about any (or all) of them? What do you think? Is it a best practice to use another type? (I might have thought about the field like isAndroid too.)
There was a problem hiding this comment.
I understand and mostly agree with the reasoning. But requiring to be explicit can lead to unnecessary frustration or repetition, especially in configurations like these. I'm almost thinking about it might be better to split android and maven targets and make them share a common core to address your concerns here.
BYK
left a comment
There was a problem hiding this comment.
Looks good with the updates so unblocking.
I'm still not sold on the "explicitly set to false" idea though. If I only care about Java packages w/o Android, why should I need to know (or worse, guess) to set android to false? Or does it even make sense setting android: false when it is actually required?
The
maventarget is responsible for releases related to Maven. Currently, the Android configuration is required, but there are scenarios where it isn't used (e.g. https://github.com/getsentry/sentry-android-gradle-plugin). This PR makes the android configuration optional.