Uh oh!
There was an error while loading. Please reload this page.
feat: support load job option ColumnNameCharacterMap - #1952
Conversation
chalmerlowe
left a comment
There was a problem hiding this comment.
LGTM.
I have one PREFERENCE to make the test code simpler and potentially easier to parse and maintain in the long run.
| config.parquet_options = None | ||
| self.assertNotIn("parquetOptions", config._properties["load"]) | ||
| def test_column_name_character_map_missing(self): |
There was a problem hiding this comment.
PREFERENCE:
These tests are nearly identical. The only real change is the choice of setting and whether the outcome matches.
This feels like an ideal situation for a parameterized test.
I would recommend we shorten this code by about 27 lines by using a pytest parameterization.
There was a problem hiding this comment.
Thanks for the suggestion! It would be really nice if we could make the tests more compact. For the purpose of this PR, I just followed the same pattern as the rest of this test file. I think we can consider revamping the test as a whole so the style of the tests can be more consistent. I do wonder though, because we are not calling the exact same methods in each of the tests (sometimes we assign value in initialization, sometimes by calling the method, or directly accessing the _properties dict), is there a nice way to represent these in test parameterization?
Fixes#1951 🦕