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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃帹 Deduplicate to_bool() implementation #53066

Open
wants to merge 2 commits into
base: devel
from

Conversation

Projects
None yet
5 participants
@webknjaz
Copy link
Member

webknjaz commented Feb 27, 2019

SUMMARY

bool parser in module_utils and bool converter in core filter plugin are loosely the same. So I've made filter plugin reuse the function from module_utils.

ISSUE TYPE
  • Refactoring Pull Request
COMPONENT NAME

lib/ansible/plugins/filter/core.py

ADDITIONAL INFORMATION

N/A

webknjaz added some commits Feb 27, 2019

@webknjaz webknjaz requested review from abadger , dagwieers and bcoca Feb 27, 2019

@webknjaz webknjaz changed the title Deduplicate to_bool() implementation 馃帹 Deduplicate to_bool() implementation Feb 27, 2019

@webknjaz webknjaz self-assigned this Feb 27, 2019

@dagwieers

This comment has been minimized.

Copy link
Member

dagwieers commented Feb 27, 2019

The implementations are not the same, so this is problematic IMO.

The old implementation considered everything lowercase that matches ('yes', 'on', '1', 'true', 1) as True, everything else is False.

The new implementation considers ('y', 'yes', 'on', '1', 'true', 't', 1, 1.0, True)) as True. So this may break stuff that expected the older behaviour.

On top of that, the old implementation would retain None values, while this does not.

I don't think we want to do this.

@dagwieers
Copy link
Member

dagwieers left a comment

The implementations are slightly different in how they handle some values (i.e. 't' and floats) and None values.

@dagwieers

This comment has been minimized.

Copy link
Member

dagwieers commented Feb 27, 2019

BTW I am not in favor of using Emojis/Unicode art in commit messages. If we start to do this we are distracting people from what matters IMO.

The index at https://github.com/ansible/ansible has been bothering me for some time with 馃毟, 馃悕, 馃崚, and emojis. I don't think it helps us in any way.

@bcoca

This comment has been minimized.

Copy link
Member

bcoca commented Feb 27, 2019

the new implementation is 'more correct' , but @dagwieers is right , this will break existing plays.

A deprecation message should be in place explaining how the current and future implementations will work, probably with a toggle strict=false|true to allow the user to control which one is active

@dagwieers

This comment has been minimized.

Copy link
Member

dagwieers commented Feb 27, 2019

@bcoca I wouldn't say considering 't' as True is more correct. I wonder where that came from.

@bcoca

This comment has been minimized.

Copy link
Member

bcoca commented Feb 27, 2019

i said 'more' but not 'totally' for a reason ...

also, i agree about emoji, they make git log look funky

@ansibot ansibot added needs_revision and removed core_review labels Feb 27, 2019

@bcoca bcoca removed the needs_triage label Mar 5, 2019

@bcoca

This comment has been minimized.

Copy link
Member

bcoca commented Mar 5, 2019

so discussed this with more core members, since this affects a commonly used filter we would prefer a deprecation notice and a transition period, with a toggle, default to current behaviour and eventually move the default to the 'common implementation

@ansibot ansibot added the stale_ci label Mar 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.