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

Fix tobool function #4647

Merged
merged 1 commit into from
Apr 11, 2024
Merged

Fix tobool function #4647

merged 1 commit into from
Apr 11, 2024

Conversation

MillhioreBT
Copy link
Contributor

Pull Request Prelude

  • I have followed [proper The Forgotten Server code styling][code].
  • I have read and understood the [contribution guidelines][cont] before making this PR.
  • I am aware that this PR may be closed if the above-mentioned criteria are not fulfilled.

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!

Copy link
Contributor

@EvilHero90 EvilHero90 left a comment

Choose a reason for hiding this comment

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

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

@MillhioreBT
Copy link
Contributor Author

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
Copy link
Member

ranisalt commented Apr 10, 2024

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

Copy link
Member

@ranisalt ranisalt left a comment

Choose a reason for hiding this comment

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

For the time being, this works and fixes an otherwise broken function. We can move the discussion to what strings to accept to a separate issue.

@EvilHero90
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor

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

@MillhioreBT MillhioreBT merged commit 1ff65ee into otland:master Apr 11, 2024
4 checks passed
@MillhioreBT MillhioreBT deleted the fix_tobool branch April 11, 2024 03:59
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.

None yet

4 participants