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

Better error checking for link and unlink commands. #202

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

benwoo1110
Copy link
Member

saveMVNPConfig() result should be used to ensure removing and adding of world link is saved to config properly.

Also, unlink command shouldn't need to check for MVWorld bcu users should be able to unlink world that they already have deleted/unloaded.

While doing this, also found a bug where it throws NPE (https://bytebin.lucko.me/1sI4jrxziC) if unlinking a toWorld that is not MVWorld which is fixed in this pr.

@benwoo1110 benwoo1110 added the Bug: Confirmed Issue/problem with the software. label Dec 11, 2020
@nicegamer7
Copy link
Member

@benwoo1110 does this build? Just wondering because MVNPConfiguration was renamed to MVNPconfiguration in two spots.

@benwoo1110
Copy link
Member Author

Yep it compiles locally for me, just rebased it with main branch as well, so there is Travis.

@nicegamer7
Copy link
Member

Awesome, gonna review soon.

@benwoo1110 benwoo1110 added PR: Bugfix Pull requests to fix bugs found in MV. and removed Bug: Confirmed Issue/problem with the software. labels Feb 9, 2021
nicegamer7
nicegamer7 previously approved these changes Apr 27, 2021
Copy link
Member

@nicegamer7 nicegamer7 left a comment

Choose a reason for hiding this comment

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

Looks good. Seems "soon" took me a while lol.

@benwoo1110
Copy link
Member Author

Just realised removeWorldLink will have api breakage, as the return type changed.

@nicegamer7
Copy link
Member

nicegamer7 commented Apr 27, 2021

Yeah I noticed too, but I kind of doubt anyone is using it...

I suppose it would be better not to break the API though.

edit: actually I don't think that's breaking, going from void to boolean. The other way around would certainly be breaking though.

@benwoo1110
Copy link
Member Author

From what I know, there isnt a way to overload return values ;(. We can have a 4.3 release 😄

@benwoo1110
Copy link
Member Author

@nicegamer7 now a new deleteWorldLink method with boolean return while deprecating the old removeWorldLink with void return. Should be good to merge :)

@benwoo1110 benwoo1110 requested a review from nicegamer7 July 3, 2021 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Bugfix Pull requests to fix bugs found in MV.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants