-
-
Notifications
You must be signed in to change notification settings - Fork 40
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
AddressParser-related changes and fixes #371
base: main
Are you sure you want to change the base?
Conversation
- Port support - Prefix support to allow IPv6 usage in modern versions
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.
Two things i noticed in first reviewable commit.
@@ -142,6 +142,8 @@ dependencies { | |||
|
|||
includeJ8("com.viaversion:viaversion:${rootProject.viaver_version}") | |||
include("com.github.TinfoilMC:ClientCommands:1.1.0") | |||
|
|||
testImplementation("org.testng:testng:6.13.1") |
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.
Why is TestNG not on 7.10.2?
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.
Gradle refused to resolve it, and that was the version IntelliJ 2022.2.2 suggested.
I can try again and see if it'll accept it later.
ProtocolVersion.v1_8), | ||
|
||
params("viafabric.v1.7.2+v1.16.5;localhost", "localhost", null, | ||
ProtocolVersion.v1_7_2, ProtocolVersion.v1_16_4), |
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.
Does 1.7.x actually work with ViaFabric? current build intentionally did not accept it.
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.
It is out of scope for AddressParser to handle I think; I just picked random versions for test cases and rolled with it to make sure it parsed correctly.
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.
It might take a while for this to be reviewed but some things;
Generally author tags for parts of the code shouldn't be done as copyright is generally backed via the license header;
Changes to the address parser/format should be explained in the README.md file as well (to make sure users know about it).
I'll write the prefix format down tomorrow, since I'm rather tired. I'm not going to write in the ability to pile up additional options tho, since that was more so mimicking the quirk of the original parser to make sure any existing invalid input in the wild would still work as expected. That can be changed easily tho if that isn't desired; i.e. just don't loop and instead bail after first iteration. |
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.
The suffix/prefix readme looks good to me.
Decided to test the suffix and it seems to work fine on my end as far as i'm aware. |
This PR has conflicts present, Please attempt to resolve them if you wish to. |
Prefix Parsing
viafabric.v1.8;localhost
is now valid. Additional meta, such as versions, can be separated by+
, like for example,viafabric.v1.18+v-2;localhost
.Note, to minimise rewriting more than is necessary, any additional versions specified after the first will be ignored, conforming to the original parser's behavior for both the new and old syntax.