add ui8 implementation introduced with UPnP 2.0 (needs Java 8)#158
add ui8 implementation introduced with UPnP 2.0 (needs Java 8)#158Jeansen wants to merge 1 commit into4thline:masterfrom
Conversation
|
Thanks for the PR. Some comments: The PR:
Why does it need Java 8? Please point me to the specific change. |
|
Note that I really don't care about formatting, import order, or any of that fluff. Removing UI4 seems questionable, however. |
|
@christianbauer for those formatting things we should consider to add a style-guide checkstyle plugin, something that automates this for us. Else every committer will have "non-sense" changes for nothing which makes PRs hard to merge later and analyze. |
|
Absolutely not, code style really doesn't matter much and arguing about it is just an excuse to look busy ;) Whatever Java IDE you use, the default code style settings are fine and close enough to the other IDEs so you won't bother anybody. Of course the same "waste of time" argument applies when you start making changes to your IDEs default code style, or you go out of your way to format a code block in two different styles, such as the members indentation in this pull requests. Back on topic: I've looked through the changes and this would break existing code that extends Cling. If it's a compatible change and requires only renaming ("use THIS instead of THAT now") we can document it and do it in a minor release. |
|
To be honest, I only did some quick refactoring - most of it done by my IDE. As you can see from the PR, I did not add much new; mostly renaming. I agree, that overwriting/removing might break some code. I am not that deep into UPnP. For me it was simply extending ui4 to ui8. Java 8 is needed because I use UnsignedIntegerEightBytes. That is only available as of Java8. At the moment I am a bit short on time. But maybe I can pull some hours aside anyhow to change to code in a way that it actually adds ui8 instead of overwriting ui4. |
|
An incompatible change in that code would open up an opportunity to rename UnsignedFoo classes and types to something shorter and maybe nicer like ui2/ui4/etc. |
In my litte hobby project I got an error accessing actions on my new router witch cling. As I see in the new UPnP 2.0 Architecture the ui8 data type was introduced. I did some refactoring and also extended a test. For me it is working now but maybe there still has some work to be done to handle the new data type correctly.