fix: sqlframe false positives in compliance checks #2011
Conversation
camriddell
changed the title
tests/translate/from_native_test.py
Outdated
There was a problem hiding this comment.
@MarcoGorelli for the coverage is there a preference among
- Add
sqlframeto the pytest GitHub CI - Remove this test
pragma: nocoverthe tests
There was a problem hiding this comment.
Just FYI, I am taking a look for how to fully support sqlframe - it's still missing a few features (not many) to pass all our tests. I would say that for now it's ok to pragma: no cover it
There was a problem hiding this comment.
definitely interested in testing sqlframe in CI, last time i checked it was too far to being passing - but they've resolved a lot of issues recently, so if we can run them in ci now then that would be great
else for now we can no cover the test
There was a problem hiding this comment.
Thanks @camriddell 🚀 Left a couple of suggestion and (maybe) addressed your question
tests/translate/from_native_test.py
Outdated
There was a problem hiding this comment.
Just FYI, I am taking a look for how to fully support sqlframe - it's still missing a few features (not many) to pass all our tests. I would say that for now it's ok to pragma: no cover it
narwhals/translate.py
Outdated
| @@ -765,6 +741,30 @@ def _from_native_impl( # noqa: PLR0915 | |||
| level="lazy", | |||
| ) | |||
|
|
|||
| # SQLFrame | |||
| # This one needs checking before extensions as `hasattr` always returns `True`. | |||
|
pending on #2011 (comment) 😉 |
|
failing downstream tests seem to be unrelated. I know that |
|
Cool thanks! pointblank also just merged a new feature, we can update its dependency on another PR |
What type of PR is this? (check all applicable)
Related issues
Checklist
If you have comments or can explain your changes, please do so below
In the
narwhals.from_nativefunction,sqlframehad to be explicitly checked before theis_compliant*checks due to false positives fromhasattr(sqlframe_df, "__narwhals_dataframe__").Instead we can use
inspect.getattr_static(thanks for the suggestion @dangotbanned) to avoid this addition and we can move thesqlframespecific code to be in line with the other backends.I also added a test to ensure
sqlframecases are handled properly.I removed a test that will break in the future, this test was passing due to a side effect caused by
hasattron apandasDataFrame enabling the use of nonhashable values as column names (raised as an issue in pandas BUG:Index.drop_duplicates()is inconsistent for unhashable values pandas-dev/pandas#60925)