-
Notifications
You must be signed in to change notification settings - Fork 21
Add getBytes() method to PlatformFile #109
base: master
Are you sure you want to change the base?
Conversation
Do you want to be a maintainer? I will ask @Wavesonics, he usually responds on Kotlin's Slack Personally I dont use this library, there was just my pet project, which used it but I have abandoned that. |
I thought about it and I'm really interested! I'm motivated to help this project grow. It will be my first time as a maintainer. I think I'll need a bit of guidance at first. Some of the questions I have:
|
That's great 🚀 Are you on Kotlin's Slack? we could disscus it there |
Yes! My name on Kotlin Slack is Vincent Guillebaud |
Any news about merging this PR ? |
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.
@vinceglb this looks good to me. I have actually been waiting for this to be merged before merging #100 as was mentioned in Slack (it might have happened before you joined), I didn't realize you were waiting on my review.
Is removing the strict visibility modifiers i.e. removing public
something that was talked about? I don't personally mind and this was done before I joined, but it was obviously added explicitly.
The By default, everything is public. If we want to keep something internal to the module, we can explicitly use the |
There is a setting to set explicit visibility modifiers which means that |
Usually it is a good practice to have them explicitly specified for libraries. There was ff5753a#diff-03ca1f7f69c9e33ebd830ae945f65c9fc879e6c2a296cedaf77e2a02e009f880 I think it should be added back |
I added back the useful method
getBytes()
that was here before toPlatFormFile
with the exact same logic as before, except for Android where I added the fix from @shubhamsinghshubham777 proposed in this PR #104 that fixes #107