Skip to content

Transport extension #85

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Transport extension #85

wants to merge 4 commits into from

Conversation

Britaliope
Copy link

@Britaliope Britaliope commented Apr 17, 2025 •
edited
Loading

closes #66

@nsowen @hoijui new MR because i can't modify the source branch of the existing one

Britaliope
Britaliope commented Apr 17, 2025
View reviewed changes
modules/core/src/main/java/com/illposed/osc/transport/OSCPortInBuilder.java
@@ -21,9 +21,10 @@ public class OSCPortInBuilder {

private OSCSerializerAndParserBuilder parserBuilder;
private List<OSCPacketListener> packetListeners;
private SocketAddress local;
private SocketAddress remote;
private SocketAddress localSocketAddress;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename of the internal variable because the setters were not named correctly, and the code checker didn't recognize them correctly and threw HiddenField alerts when using this.<varname> = <varname>; instead of arbitrary different name.

(same for remote address, and same for PortOutBuilder class)

modules/parent/src/main/resources/checkstyle.xml
@@ -189,6 +189,7 @@ SPDX-License-Identifier: CC0-1.0
<module name="HiddenField">
<property name="ignoreConstructorParameter" value="true"/>
<property name="ignoreSetter" value="true"/>
<property name="setterCanReturnItsClass" value="true" />
Copy link
Author

@Britaliope Britaliope Apr 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Helps the codestyle checker to correctly recognize setters that return the class.

@Britaliope
Copy link
Author

Britaliope commented Apr 17, 2025 •
edited
Loading

Notes for review:

  • I haven't been able to run the tests on my machine (some weird issues about Assert not being recognized, i'm not really used to the java ecosystem and haven't been able to fix it). I tested on my very simple app that use this and it works, but if someone else can run the tests to see if everything is fine it would be great.

  • I changed the setter style to this.<varname> = <varname> instead of arbitrary different name into the whole OSCPort{In,Out}Builder classes for consistency. This cascaded into a rename of some private variables (see the full explaination above) which makes the full diff a bit messy to read. It is in a separate commit so you can review things separately.

  • I rebased (not merged, that's the workflow i'm used to) the branch on hoijui/master and fixed the conflicts. I can re-do this with a merge if you prefer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Britaliope @nsowen