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

Hotfix: Fix version strings pickup up unneeded input. #4282

Merged
merged 19 commits into from Sep 10, 2020
Merged

Hotfix: Fix version strings pickup up unneeded input. #4282

merged 19 commits into from Sep 10, 2020

Conversation

ghost
Copy link

@ghost ghost commented Aug 21, 2020

Description:

Fixes issue where players using the version: "x.x.x.x-for-x.x.x" would receive errors because the version string was not sanitized.


  • I have tested this pull request for defects on a server.

By making this pull request, I represent that I have the right to waive copyright and related rights to my contribution, and agree that all copyright and related rights in my contributions are waived, and I acknowledge that the TownyAdvanced organization has the copyright to use and modify my contribution under the Towny License for perpetuity.

@ghost
Copy link
Author

ghost commented Sep 6, 2020

This has been mergable for a bit now

@LlmDl
Copy link
Member

LlmDl commented Sep 6, 2020

Yeah we were looking at it and it seems a bit overkill. Changing things to versions just to get a string from it in the end. The original problem really only needs to have the non-number bits stripped off it like you did with the pattern matcher.

@ghost
Copy link
Author

ghost commented Sep 6, 2020

Yeah we were looking at it and it seems a bit overkill. Changing things to versions just to get a string from it in the end. The original problem really only needs to have the non-number bits stripped off it like you did with the pattern matcher.

It is getting stripped, the reason I'm using Version instead of String in many areas is because the version class allows us to do comparisons between versions you can't compare versions if they're just in a string format. For example if you're given two version strings you can't easily compute which one is less than/greater than.

@silverwolfg11
Copy link
Contributor

Is there a way to abstract a lot of the visible string parsing out. I don't mind using the Version class to better check version comparison if that's more effective. It just seems like all the string parsing like .split to get the the proper format to plug into the version can be done a bit more clean.

Maybe instead of having the pattern in string mgmt you move it to the version class and then you have a static like Version.parse() method?

Furthermore I'm noticing that TownySettings.getRunLastVersion() also needs a lot of string manipulation. Is there a way to create a method in TownySettings that contains the major use-case and forwards onto the actual getRunLastVersion like maybe a method by the same name that has no parameters but forwards to TownySettings.getRunLastVersion("0.0.0.0").

Just stuff to make it more clean in general.

src/com/palmergames/bukkit/towny/Towny.java Outdated Show resolved Hide resolved
src/com/palmergames/bukkit/util/BukkitTools.java Outdated Show resolved Hide resolved
src/com/palmergames/bukkit/towny/TownySettings.java Outdated Show resolved Hide resolved
src/com/palmergames/bukkit/towny/TownySettings.java Outdated Show resolved Hide resolved
src/com/palmergames/bukkit/util/BukkitTools.java Outdated Show resolved Hide resolved
src/com/palmergames/bukkit/towny/TownyUniverse.java Outdated Show resolved Hide resolved
@ghost ghost requested a review from LlmDl September 7, 2020 18:11
@LlmDl
Copy link
Member

LlmDl commented Sep 7, 2020

I'll do some further testing on this later, thanks.

@LlmDl LlmDl merged commit 2e5a816 into TownyAdvanced:master Sep 10, 2020
LlmDl added a commit that referenced this pull request Sep 10, 2020
issues with config migration, courtesy of Siris with PR #4282.
@ghost ghost deleted the hotfix/version-strings branch September 11, 2020 05:03
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