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

add ui8 implementation introduced with UPnP 2.0 (needs Java 8) #158

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Jeansen
Copy link

@Jeansen Jeansen commented Jan 13, 2016

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.

@ened
Copy link
Contributor

ened commented Mar 16, 2016

Thanks for the PR. Some comments:

The PR:

  • Changes the import orders
  • Imports a few classes which are not needed
  • Changes code formatting
  • Changes visibility of some of the classes (Builtin enum)
  • Removes any support for ui4. Does UPnP2 also remove ui4?

Why does it need Java 8? Please point me to the specific change.

@christianbauer
Copy link
Member

Note that I really don't care about formatting, import order, or any of that fluff. Removing UI4 seems questionable, however.

@ened
Copy link
Contributor

ened commented Mar 16, 2016

@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.

@christianbauer
Copy link
Member

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.

@Jeansen
Copy link
Author

Jeansen commented Mar 17, 2016

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.

@christianbauer
Copy link
Member

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.

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