Skip to content

refactor: allow callable in CompliantExpr.from_column_names to reuse for CompliantNamespace.<all|col|exclude>#2134

Merged
FBruzzesi merged 8 commits intomainfrom
refactor/from_column_names
Mar 3, 2025
Merged

refactor: allow callable in CompliantExpr.from_column_names to reuse for CompliantNamespace.<all|col|exclude>#2134
FBruzzesi merged 8 commits intomainfrom
refactor/from_column_names

Conversation

@FBruzzesi
Copy link
Member

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

  • 💾 Refactor
  • ✨ Feature
  • 🐛 Bug Fix
  • 🔧 Optimization
  • 📝 Documentation
  • ✅ Test
  • 🐳 Other

Related issues

Follow-up on comment

@dangotbanned
Copy link
Member

Apologies for the stream-of-consciousness @FBruzzesi 😉

@FBruzzesi FBruzzesi changed the title refactor: tweak CompliantExpr.from_column_names to reuse in CompliantNamespace.exclude refactor: tweak CompliantExpr.from_column_names to reuse in CompliantNamespace.<all|col|exclude> Mar 2, 2025
@FBruzzesi FBruzzesi changed the title refactor: tweak CompliantExpr.from_column_names to reuse in CompliantNamespace.<all|col|exclude> refactor: allow callable in CompliantExpr.from_column_names to reuse for CompliantNamespace.<all|col|exclude> Mar 2, 2025
@FBruzzesi
Copy link
Member Author

FBruzzesi commented Mar 2, 2025

Apologies for the stream-of-consciousness @FBruzzesi 😉

No worries! There are a lot of good points and I as well realized all could be refactored with the same approach! I should have opened this as draft to begin with.

I am quite happy with it now - I should have addressed all your points except comment for which I have mixed feelings.

@dangotbanned dangotbanned self-requested a review March 3, 2025 09:37
- `*column_names` was already positional-only
- The `from_column_names` name is already explicit enough in what the first argument should be IMO
Copy link
Member

@dangotbanned dangotbanned left a comment

Choose a reason for hiding this comment

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

Thanks for this @FBruzzesi

I made two tweaks that I hadn't thought of yet in:

I'm approving but feel free to merge without those included, if you prefer 🙂

@FBruzzesi
Copy link
Member Author

Thanks @dangotbanned 🙌🏼

@FBruzzesi FBruzzesi merged commit b9d4529 into main Mar 3, 2025
27 of 28 checks passed
@FBruzzesi FBruzzesi deleted the refactor/from_column_names branch March 3, 2025 18:29
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.

2 participants