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

Extend is_yes() #5273

Closed
wants to merge 1 commit into from
Closed

Extend is_yes() #5273

wants to merge 1 commit into from

Conversation

red-001
Copy link
Contributor

@red-001 red-001 commented Feb 18, 2017

This is used by Settings::getBool so it would make sense for it to return true when passed on.

for:

  • rubenwardy
  • nerzhul

against

  • sfan5
  • celeron55
  • zeno ?

@rubenwardy
Copy link
Member

👍

@sfan5
Copy link
Member

sfan5 commented Feb 19, 2017

No it would not, this pull was already rejected last time you made it (~6 weeks ago)

http://irc.minetest.net/minetest-dev/2017-01-06#i_4780321 // #5002

@sfan5 sfan5 closed this Feb 19, 2017
@red-001
Copy link
Contributor Author

red-001 commented Feb 19, 2017

Consider that that one also had one approval that should add up to two approvals. Shouldn't it at least be reconsidered? Plus I though your issue with it was that I hasn't had gotten two approvals?

@rubenwardy
Copy link
Member

I think this makes sense as it makes it consistent with eg: cmake

@paramat
Copy link
Contributor

paramat commented Feb 19, 2017

But is 'on' needed for something? best that settings avoid 'on' and 'off'.
If not -1.

@rubenwardy
Copy link
Member

tbh it's not that useful in settings, more for mods and UIs. So this PR can be worked around.

tbh, it's not worth debating this - it's such a minor thing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants