Map Device List#76
Conversation
|
Hey, thanks for the contribution. I really like this new api design. Can we deprecate the old one and only allow the new one to be used? Also, can we only allow one of the two to be set from the fladle extension? (Throw an exception if both are set) |
|
@runningcode sure thing! I'll add the deprecation and force only one to be used in the extension. |
|
@runningcode Do you think it would be better to simply replace Thoughts? |
|
Yeah, you know what? Fladle is still a 0.x library so we reserve the right to make these changes. Let's go ahead and do that. I think the importing of the |
|
Sounds good to me, will change the minor version to |
…ther than requiring the Device class to be referenced in Gradle.
…apDevices" first, then "devices"
|
@runningcode Done deal! I figured bumping to |
runningcode
left a comment
runningcode
left a comment
There was a problem hiding this comment.
Looking good! Just a small nit.
| appendln(" - model: ${device.model}") | ||
| appendln(" version: ${device.version}") | ||
| device.orientation?.let { | ||
| val model = device["model"] |
There was a problem hiding this comment.
| val model = device["model"] | |
| val model = device["model"]!! |
I believe model and version are required fields. Rather than printing - model: null, let's fail fast here.
Maybe we can write a test for this case too.
There was a problem hiding this comment.
Good call, will fix this 👍
There was a problem hiding this comment.
Alright, I added a RequiredDeviceKeyMissingException and throw that when "model" or "version" are null: 4764435
|
|
||
| @VisibleForTesting | ||
| internal fun createDeviceString(devices: List<Device>): String = buildString { | ||
| internal fun createDeviceString(devices: List<Map<String, String?>>): String = buildString { |
There was a problem hiding this comment.
About to merge, but I need to talk this out. :) Is it possible for the value to be null here? For example if we have orientation in the map, the value for orientation must be non-null. Same goes for model?
There was a problem hiding this comment.
You know what, that's a good point haha. I'll remove the ? that's unnecessary
|
Thanks for helping out and fixing all those nitpicks. I've realized a lot of people are using this library so we want to make this API as nice as possible. |
|
Happy to help! It's a great plugin to use alongside Flank :) |
3 participants
Updated
devicesto support a different API using a list of maps for setting up devices rather than requiring theDeviceclass to be referenced in Gradle.In reference to: #31
Usage:
fladle { ... devices = [ /* No longer allowed */ // new Device("Nexus5", 28) /* Use this instead */ [ "model": "Nexus5", "version": "28" ] ] }