Skip to content

Conversation

xclaesse
Copy link
Member

No description provided.

@xclaesse xclaesse requested a review from jpakkane as a code owner October 15, 2025 01:30
@dcbaker
Copy link
Member

dcbaker commented Oct 15, 2025

This is a behavior change that is absolutely going to break someone's build.

And of course, there isn't an obvious way (in my mind) to fix this because of the way disablers are handled.

@xclaesse
Copy link
Member Author

It's actually not a breaking change because is_disabler([foo, bar]) already fails because it accepts only 1 positional argument.

@xclaesse
Copy link
Member Author

The use-case here is something like that:

list = [foo, bar]
if something
  list = disabler()
endif
...
if is_disabler(list) # This used to fail because list is flatten into `is_disabler(foo, bar)` and that func allows a single argument
  ...
endif

@xclaesse
Copy link
Member Author

Note there is a subtle workaround: is_disabler(list.lenght()), because any method call on a disabler does return a disabler.

@dcbaker
Copy link
Member

dcbaker commented Oct 15, 2025

Oh, well if it already fails because the list gets flattened, then that seems fine. I retract my objections.

Some docs would be nice, but otherwise seems fine.

@xclaesse
Copy link
Member Author

Added doc for the behavior with a list argument. Also added a FeatureNew.

@eli-schwartz
Copy link
Member

It appears this change breaks a test that explicitly assumes summary() can take a dictionary with elements that are disablers.

is_disabler() must not flatten its arguments otherwise
is_disabler([foo, bar]) gets converted to is_disabler(foo, bar) and we
allow only a single positional argument.
@xclaesse
Copy link
Member Author

It appears this change breaks a test that explicitly assumes summary() can take a dictionary with elements that are disablers.

Hm, I fear this change could have too many unforeseen consequences like that. I changed this PR to not change the behavior of is_disabled() (can be revisited later) and make the public function is_disabler() reflects whatever is_disabled() does with lists/dicts. My initial goal here was really to make them both consistent.

@xclaesse
Copy link
Member Author

xclaesse commented Oct 17, 2025

I intentionally left is_disabler() doc not mention what happens with dictionaries to not close the door to future changes.

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.

3 participants