-
-
Notifications
You must be signed in to change notification settings - Fork 20
feat(maven): Optional android config
#271
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
Merged
+151
−33
Merged
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
d771cb0
Make Android config optional
iker-barriocanal 5bb6004
Update README
iker-barriocanal e4dc758
Update changelog
iker-barriocanal b95af38
Make latest changelog entries `feat`
iker-barriocanal c0691ef
Set required android config
iker-barriocanal d5c09ca
Update tests
iker-barriocanal 2be40a7
Update docs
iker-barriocanal 72a9e50
Merge branch 'master' into iker/ref/maven-android-optional
iker-barriocanal 960a415
feedback
iker-barriocanal File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not
nullorundefined? I'm wary about variable-type fields.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thinking about what this config field should answer is "I don't want to configure Android".
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.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.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
falseseems 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 likeisAndroidtoo.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
androidandmaventargets and make them share a common core to address your concerns here.