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

First PR cleanup #18

Merged
merged 4 commits into from
Jan 31, 2025
Merged

First PR cleanup #18

merged 4 commits into from
Jan 31, 2025

Conversation

connortsui20
Copy link
Member

Problem

#4 Was the first major PR, but since we were making a lot of changes last minute there were a few things that slipped through with respect to quality control.

Summary of changes

  • Added very strict clippy documentation requirements. Discussion below.
  • Added documentation for everything except the operator::relational::physical module and the operator::scalar module, as those seem kind of unstable right now.
  • Changed the scalar generic param of expressions to use ScalarGroupId instead of GroupId.
  • Added several TODOs that I think will be resolved pretty quickly, though I'd appreciate it if others took a look at those in the case that we can actually get those done now.

Now that we have something to work off of, requiring documentation on everything (including private items) means that we'll pay the cost right now of making sure people understand what we're doing in the future. I'm willing to relax it a little bit if we put other stuff in place that ensure nobody in the future encounters large chunks of code with zero documentation.

@connortsui20 connortsui20 changed the title First pr cleanup First PR cleanup Jan 30, 2025
@codecov-commenter
Copy link

codecov-commenter commented Jan 30, 2025

@AlSchlo
Copy link
Collaborator

AlSchlo commented Jan 31, 2025

Perhaps we should rename logical scan to "get". Scan semantically sounds more like a physical implementation.

@connortsui20
Copy link
Member Author

Perhaps we should rename logical scan to "get". Scan semantically sounds more like a physical implementation.

If we're going to rename it at all, it should probably be named Read instead of Get, but even so I think Scan is more universal and it will be clear what we're all talking about when we mention it.

@connortsui20 connortsui20 merged commit 3962965 into main Jan 31, 2025
12 checks passed
@connortsui20 connortsui20 deleted the first-pr-cleanup branch January 31, 2025 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants