otland / forgottenserver

A free and open-source MMORPG server emulator written in C++
https://otland.net
GNU General Public License v2.0
1.57k stars 1.05k forks source link

Fix tobool function #4647

Closed MillhioreBT closed 4 months ago

MillhioreBT commented 5 months ago

Pull Request Prelude

Changes Proposed

the tobool function was not getting any characters from the str parameter, so it would always return false In this case the correct string.sub method is being used and the correct index is 1

Issues addressed: Nothing!

MillhioreBT commented 4 months ago

This needs rather an entire re work, it's not like bools can only consist of true Also we should do strict comparison and not by just y because str could be equal to year and it would still return as true in this case

What do you suggest, why is this better than nothing, the current code is broken, it cannot index a string with the [] operator

ranisalt commented 4 months ago

it's not like bools can only consist of true

This function accepts any word that starts with 1yYtT, which is consistent with other languages, i.e. Python and Go. I don't think we need to guard against stupidity, but it has been raised in the past

It doesn't accept on though (like Python, unlike Go) which could be an option, and doesn't reject unknown values

EvilHero90 commented 4 months ago

it's not like bools can only consist of true

This function accepts any word that starts with 1yYtT, which is consistent with other languages, i.e. Python and Go. I don't think we need to guard against stupidity, but it has been raised in the past

It doesn't accept on though (like Python, unlike Go) which could be an option, and doesn't reject unknown values

Well what's the purpose of fixing something for the sake of just fixing it halfway that we open up another pr to fully fix it? The other concern I have is the placement of this function, it's currently placed in xml.lua which implies for me that it is meant for xml bools only, if we want to make it reactive to all different kind of forms (even support Go and Python) then we should consider moving that to a more descriptive file

ranisalt commented 4 months ago

The other concern I have is the placement of this function, it's currently placed in xml.lua which implies for me that it is meant for xml bools only

Good point, it was a support function for loading XML but if it's used elsewhere, it should indeed be moved. I agree with all your points, I would just do one step at a time, but I would approve a major fix that checks all boxes 😉

EvilHero90 commented 4 months ago

Good point, it was a support function for loading XML but if it's used elsewhere, it should indeed be moved. I agree with all your points, I would just do one step at a time, but I would approve a major fix that checks all boxes 😉

alright sounds fine to me