Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: factor out check_columns_exist #1792

Merged
merged 5 commits into from
Jan 11, 2025

Conversation

DeaMariaLeon
Copy link
Member

What type of PR is this? (check all applicable)

  • πŸ’Ύ Refactor
  • ✨ Feature
  • πŸ› Bug Fix
  • πŸ”§ Optimization
  • πŸ“ Documentation
  • βœ… Test
  • 🐳 Other

Related issues

Checklist

  • Code follows style guide (ruff)
  • Tests added
  • Documented the changes

If you have comments or can explain your changes, please do so below

@@ -1059,3 +1059,9 @@ def generate_repr(header: str, native_repr: str) -> str:
"| Use `.to_native` to see native output |\nβ””"
f"{'─' * 39}β”˜"
)


def check_column_exists(columns: list[str], subset: str | list[str] | None) -> None:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't subset need to be a list here?

Suggested change
def check_column_exists(columns: list[str], subset: str | list[str] | None) -> None:
def check_column_exists(columns: list[str], subset: list[str] | None) -> None:

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like this is wrong in _spark_like actually - could you correct that one from

subset: str | list[str] | None = None,

to

subset: list[str] | None = None,

please?

Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @DeaMariaLeon !

@DeaMariaLeon DeaMariaLeon merged commit 1f22a1d into narwhals-dev:main Jan 11, 2025
22 of 23 checks passed
@DeaMariaLeon DeaMariaLeon deleted the check branch January 11, 2025 16:30
@MarcoGorelli
Copy link
Member

as another follow-up, we could show which columns are missing?

like

if subset is not None and (missing := set(subset).difference(columns)):
    # raise a message showing that `missing` columns are not found

?

@DeaMariaLeon
Copy link
Member Author

Yeah, sure @MarcoGorelli. Did I close this too soon? 😱

@MarcoGorelli
Copy link
Member

πŸ˜„ no i meant in another pr, if you fancy it

@DeaMariaLeon
Copy link
Member Author

Sure, sure! πŸ™‚

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

chore: factor out check_columns_exist
2 participants