Skip to content

Do not remove empty values in a non-empty Array.#38

Draft
joshuapinter wants to merge 1 commit intonathanl:masterfrom
joshuapinter:do_not_remove_empty_values_in_non_empty_array
Draft

Do not remove empty values in a non-empty Array.#38
joshuapinter wants to merge 1 commit intonathanl:masterfrom
joshuapinter:do_not_remove_empty_values_in_non_empty_array

Conversation

@joshuapinter
Copy link
Copy Markdown

Hey @nathanl,

Wanted to get your feedback on this.

After upgrading from v3 to v4, we noticed some of our searches were not working.

We use an Array for date ranges, first element is Start Date and second element is End Date. We also allow for open-ended date ranges by leaving one of those nil.

So it would look like this for >= Jan 1, 2024:

options[ :created_between ] = [ "Jan 1, 2024", nil ]

In v3 and before, that nil value would be left in there.

In v4, that nil value is being removed and the Array becomes [ "Jan 1, 2024" ].

This occurs in the excluding_empties method:

if value.instance_of?(Array)
  output[key] = value.reject { |v| empty?(v) }
end

Our thinking is to only reject an Array that is completely empty or filled with nil or empty Strings "".

We forked it to look something like this:

if value.instance_of?(Array)
  output[key] = nil if output[key].all?(&:blank?) # If Array is entirely empty (i.e. `nil` or empty Strings only) then remove it.
end

Two things:

  1. Was this an intentional change from v3 to v4? If not, we can restore how it worked.
  2. If this was intentional then would you be open to a config to change the behaviour here and to avoid removing blank? values from a non-blank Array?

We're using our fork for now so no rush but let me know your thoughts.

Thanks.

FIXME: Make this an option or configurable.
@joshuapinter joshuapinter marked this pull request as draft January 26, 2024 18:00
@nathanl
Copy link
Copy Markdown
Owner

nathanl commented Jan 28, 2024

Hey Joshua. Glad you figured out the problem you're having. Given that the last release was several years ago and I haven't used Ruby in about as long, I'm very hesitant to change anything at this point.

I suggest you either keep using your fork (I doubt this repo will ever change) or else use a value like :empty instead of nil. What do you think?

@nathanl
Copy link
Copy Markdown
Owner

nathanl commented Jan 28, 2024

I honestly don't remember the reason for this change but I assume others are relying on the v4 behavior.

@joshuapinter
Copy link
Copy Markdown
Author

joshuapinter commented Jan 29, 2024

Totally fair and I really appreciate the response, regardless. I know what it's like keeping legacy open source projects around. Ha.

I think we'll continue using our fork for now, especially since this lib won't be changing so it's not like we're missing out on "upstream" changes or version bumps.

If we circle back to this and have more time to put in to keep it updated, etc I'll let you know and maybe we can take it off your hands, maintenance-wise.

Gonna leave this open as a record in case it catches anybody else outs, if you're okay with that.

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.

2 participants