Skip to content

document internal/logger and internal/utils#208

Merged
gflarity merged 2 commits into
ai-dynamo:mainfrom
gflarity:gflarity/document_internal_logger_and_utils
Oct 22, 2025
Merged

document internal/logger and internal/utils#208
gflarity merged 2 commits into
ai-dynamo:mainfrom
gflarity:gflarity/document_internal_logger_and_utils

Conversation

@gflarity

@gflarity gflarity commented Oct 2, 2025

Copy link
Copy Markdown
Contributor

What type of PR is this?

/documentation

What this PR does / why we need it:

Improves documentation the in internal/logger and internal/utils folders. Similar the documentation side of #204

Special notes for your reviewer:

Here's the prompt used:

Please create an idiomatic golang unit tests for this specific directory/package. Herea are the instructions:

  • Start by revieiwng the state of test coverage with golang tooling.
  • Be sure to document the fields in the anonymous test structs and how they'll be used below.
  • Each Test* function should also have some concise documentation as well. Each test case should have a brief explanation and expectation explaine. This should be right before the go struct definition for that test if it's table driven tests.
  • Each test should have a name field for cross referencing failures.
  • Rather than having a description field, the description should just be in comments above the name in the struct declaration.
  • Don't forget to run the tests and confirm they're working, but focus on just testing this package as others might have issues.
  • Please avoid creating skipped tests.
  • Refactor code to make it more testable if necessary. However keep these refactors to a minimum and as simple as possible.
  • Specifically hardcoded constants etc are good candidates to be refactor out of testable code. As are interface that can be mocked in a straight forward way. However you must remember to ensure backwards compatibility and new functions rather than changing the signature of public functions. Avoid making these test helpers public though. Only make such refactors when there's a good return on investment.
  • Please ensure coverage is as good as possible using golang tooling.
  • Avoid creating table driven tests when the test function just has a single test in it.
  • Avoid test that don't really test anything.
  • Avoid describing tests as edge cases.
  • Avoid tests for command line arguments. Try to keep test cases unique and adding value (avoid duplicate test cases).

Does this PR introduce a API change?

NONE

@gflarity gflarity requested a review from renormalize October 2, 2025 15:21
@gflarity gflarity force-pushed the gflarity/document_internal_logger_and_utils branch from 2c5f050 to 5280cd6 Compare October 3, 2025 16:41
Ronkahn21
Ronkahn21 previously approved these changes Oct 16, 2025
renormalize
renormalize previously approved these changes Oct 21, 2025

@renormalize renormalize left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There seems to be a few common comments in #211 and this PR. Just a heads-up since you might get conflicts in the other after merging one!

Thanks for the PR!

@gflarity gflarity force-pushed the gflarity/document_internal_logger_and_utils branch from 5280cd6 to 4e19ea0 Compare October 22, 2025 20:10
Signed-off-by: Geoff Flarity <gflarity@nvidia.com>
Signed-off-by: Geoff Flarity <gflarity@nvidia.com>
@gflarity gflarity dismissed stale reviews from renormalize and Ronkahn21 via a5af555 October 22, 2025 20:24
@gflarity gflarity force-pushed the gflarity/document_internal_logger_and_utils branch from 4e19ea0 to a5af555 Compare October 22, 2025 20:24
@gflarity gflarity merged commit 38f06f1 into ai-dynamo:main Oct 22, 2025
4 checks passed
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