potential fix for boolean elem alteration

[jlloyd-widen]

I have run into an error/bug where an element from a row is "false" and the schema for the field is ['null', 'string', 'boolean']. When this happens the is_boolean_type logic will trigger from line 219 of _typing.py but the if/then logic ends up executing lines 225 and 226 which changes the value from the original "false" to True. Obviously no bueno!

I'm not necessarily dead set on this being the solution. It's just the easiest one to implement for my particular case. Happy to take feedback or suggestions.

[codecov]

Codecov Report

Merging #929 (beac08d) into main (0b2fe9e) will decrease coverage by 6.50%.
The diff coverage is 0.00%.

❗ Current head beac08d differs from pull request most recent head a040acb. Consider uploading reports for the commit a040acb to get more accurate results

@@            Coverage Diff             @@
##             main     #929      +/-   ##
==========================================
- Coverage   86.97%   80.48%   -6.50%     
==========================================
  Files          59       35      -24     
  Lines        5091     3484    -1607     
  Branches      825      696     -129     
==========================================
- Hits         4428     2804    -1624     
- Misses        466      507      +41     
+ Partials      197      173      -24     

Files Changed	Coverage Δ
singer_sdk/helpers/_typing.py	45.25% <0.00%> (-25.23%)	⬇️

... and 61 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[aaronsteers]

Do we need to have support for alternate spellings / casing?

Suggested change

	elif elem in [0, 'false']:
	elif elem in [0, 'false', 'False', 'FALSE']:

[aaronsteers]

My gut says stringification of arbitrary values could have a minor perf impact. What about putting the string check behind a type check?

Suggested change

	elif elem in [0, 'false']:
	elif elem == 0:
	boolean_representation = False
	elif isinstance(elem, str) and elem == 'false':
	boolean_representation = False

@edgarrmondragon - thoughts?

[edgarrmondragon]

@aaronsteers 👍 and I like @jlloyd-widen's iteration over this to make it case insensitive

[edgarrmondragon]

Also, I just did some quick benchmarks and isinstance seems to cause execution of this block to more than double (129.7707ns vs 289.6716ns on my machine).

[aaronsteers]

Looks good from my side. I asked about alternate spellings, but nothing too much at issue there.

[aaronsteers]

My gut says stringification of arbitrary values could have a minor perf impact. What about putting the string check behind a type check?

Suggested change

	elif elem in [0, 'false']:
	elif elem == 0:
	boolean_representation = False
	elif isinstance(elem, str) and elem == 'false':
	boolean_representation = False

@edgarrmondragon - thoughts?

[jlloyd-widen]

@aaronsteers Good ideas. I just pushed a new commit that's a bit more comprehensive. Let me know if you or @edgarrmondragon have more thoughts

[edgarrmondragon]

@jlloyd-widen what happens if the value is a string but not one of the checked options?

[jlloyd-widen]

then it would force it to true ... so that's not good either

[edgarrmondragon]

@jlloyd-widen even if True was an acceptable value, it seems to me that boolean_representation wouldn't have a value because there's no else here?

[jlloyd-widen]

@edgarrmondragon just added a new commit. I tried to make the logic inclusive of true scenarios. Where the input element isn't obviously true or false, I pass it along as the final property value unaltered.

[tayloramurphy]

@jlloyd-widen this is a pretty old PR at this point. Do you still want to try and get this merged? cc @edgarrmondragon

[jlloyd-widen]

I just synced the most recent code with this branch and found that the strategy now being used for this logic is significantly different than when I originally submitted the PR. So I'm going to assume this is no longer needed and close this PR.