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

Clean up doc comments and add periods #43

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

connortsui20
Copy link
Member

Problem

Cleans up a bit of the CPS PR.

Summary of changes

This commit adds periods to all of the comments in the cir and engine. I also added some more detail here and there.

This commit also unifies all of the types in the cir to make it easier to interact with those types. Imo it makes more sense to have them all in one place rather than to have to figure out the paths to each of them every time you import.

This commit adds periods to all of the comments in
the cir and engine.

This commit also unifies all of the types in the
cir to make it easier to interact with those
types.
@codecov-commenter
Copy link

codecov-commenter commented Mar 14, 2025

Codecov Report

Attention: Patch coverage is 75.30864% with 20 lines in your changes missing coverage. Please review.

Project coverage is 73.3%. Comparing base (60c275f) to head (0f36b12).

Files with missing lines Patch % Lines
optd-core/src/bridge/into_cir.rs 0.0% 11 Missing ⚠️
optd-core/src/bridge/from_cir.rs 0.0% 3 Missing ⚠️
optd-core/src/cir/expressions.rs 0.0% 2 Missing ⚠️
optd-core/src/cir/plans.rs 0.0% 2 Missing ⚠️
optd-core/src/engine/eval/binary.rs 75.0% 2 Missing ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
optd-core/src/cir/rules.rs 0.0% <ø> (ø)
optd-core/src/engine/eval/core.rs 100.0% <100.0%> (ø)
optd-core/src/engine/eval/expr.rs 95.1% <100.0%> (ø)
optd-core/src/engine/eval/match.rs 99.0% <100.0%> (ø)
optd-core/src/engine/eval/mod.rs 96.6% <ø> (ø)
optd-core/src/engine/eval/operator.rs 100.0% <100.0%> (ø)
optd-core/src/engine/eval/unary.rs 80.8% <100.0%> (ø)
optd-core/src/engine/mod.rs 6.4% <100.0%> (ø)
optd-core/src/engine/test_utils.rs 100.0% <100.0%> (ø)
optd-core/src/engine/utils.rs 100.0% <ø> (ø)
... and 10 more
🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

///
/// # Arguments
/// * `children` - Collection of child items to convert
/// A generic function that converts children with type `S` into a `Arc<T>`.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I prefer the old comment here.

Copy link
Contributor

Choose a reason for hiding this comment

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

the eg. of what S and T can be is helpful

Copy link
Member Author

Choose a reason for hiding this comment

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

In general I dislike comments that explain the function signature essentially verbatim, they don't really add anything in helping someone understand why this function is here and ends up cluttering headspace.

@connortsui20 connortsui20 requested a review from SarveshOO7 March 15, 2025 02:15
@connortsui20 connortsui20 force-pushed the connor/clean-cps-docs branch from 7e176c5 to 0f36b12 Compare March 15, 2025 02:16
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.

3 participants