Skip to content
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

Android changes #23

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Android changes #23

wants to merge 5 commits into from

Conversation

psiniemi
Copy link

To run on older android devices, I needed to remove references to Byte.toUnsignedInt and newer javas also don't ship with javax.xml anymore, so I also removed javax.xml.bind.DatatypeConverter which was only used for bytes -> hex conversion and replaced with a HexUtil class that can do the conversion both ways.

@a1aw
Copy link
Owner

a1aw commented Aug 12, 2019

Thanks for the modification. Would it be better to change the static import directly as import com.github.mob41.blapi.HexUtil and use HexUtil.byte2hex? Or is it specially used?

@a1aw
Copy link
Owner

a1aw commented Aug 12, 2019

Your pull request has a build error, which is caused by invalid maven pom.xml configuration: https://travis-ci.org/mob41/broadlink-java-api/builds/570753996

Removing the changes or fixing the invalid characters can help. Btw, what is the source configuration is for?

@flo-02-mu
Copy link

Is there any chance of getting this PR included? The ability to use the library with java 11 would be super nice. I added a PR onto the PR to fix the pom-file (https://github.com/psiniemi/broadlink-java-api/pulls) but no feedback so far.

@a1aw
Copy link
Owner

a1aw commented Dec 5, 2019

Unless author accepts your PR or makes changes to his code, I couldn’t merge two branches together cause it could break changes.

Remove incorrect characters from pom file
@psiniemi
Copy link
Author

psiniemi commented Dec 6, 2019

Sorry, completely forgot about this and somehow missed the notifications until now. Will look into the comments this weekend.

@codecov
Copy link

codecov bot commented Dec 7, 2019

Codecov Report

Merging #23 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #23   +/-   ##
=====================================
  Coverage       0%     0%           
=====================================
  Files          42     43    +1     
  Lines        1013   1024   +11     
  Branches      107     92   -15     
=====================================
- Misses       1013   1024   +11
Impacted Files Coverage Δ
...rc/main/java/com/github/mob41/blapi/SP2Device.java 0% <0%> (ø) ⬆️
src/main/java/com/github/mob41/blapi/A1Device.java 0% <0%> (ø) ⬆️
...ub/mob41/blapi/pkt/cmd/hysen/BaseHysenCommand.java 0% <0%> (ø) ⬆️
src/main/java/com/github/mob41/blapi/HexUtil.java 0% <0%> (ø)
...rc/main/java/com/github/mob41/blapi/SP1Device.java 0% <0%> (ø) ⬆️
src/main/java/com/github/mob41/blapi/BLDevice.java 0% <0%> (ø) ⬆️
...rc/main/java/com/github/mob41/blapi/MP1Device.java 0% <0%> (ø) ⬆️
...rc/main/java/com/github/mob41/blapi/RM2Device.java 0% <0%> (ø) ⬆️
.../github/mob41/blapi/dev/hysen/BaseHysenDevice.java 0% <0%> (ø) ⬆️
...ain/java/com/github/mob41/blapi/pkt/CmdPacket.java 0% <0%> (ø) ⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b5cc9c5...66c3f60. Read the comment docs.

@psiniemi
Copy link
Author

psiniemi commented Dec 9, 2019

Hi, static imports are pretty much a question of taste. The only downside I see is that if there are a lot of them, it can make following the code quite confusing. I like to use them as it makes the code shorter and as long as the name of the function name says what it does, I find it easier to follow. But since it appears our tastes differ, I removed the static import.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants