Skip to content

ValidationScheme implements pflag.Value and json.Marshaler/Unmarshaler interfaces #807

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

juliusmh
Copy link

No description provided.

@juliusmh juliusmh force-pushed the jmh/validation_scheme_pflag_json branch from 70453fd to 06cb330 Compare July 31, 2025 11:47
Copy link
Contributor

@aknuds1 aknuds1 left a comment

Choose a reason for hiding this comment

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

I don't understand why you're removing the fmt.Stringer assertion.

@aknuds1 aknuds1 requested a review from Copilot July 31, 2025 11:56
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements additional interfaces for the ValidationScheme type to support JSON marshaling/unmarshaling and command-line flag parsing. The changes enhance the ValidationScheme type by making it compatible with JSON serialization and pflag.Value interface for CLI usage.

Key changes:

  • Added JSON marshaling and unmarshaling support for ValidationScheme
  • Implemented pflag.Value interface methods (Set and Type)
  • Refactored existing YAML unmarshaling to use the new Set method for consistency

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
model/metric.go Implements JSON marshaling/unmarshaling and pflag.Value interface methods for ValidationScheme
model/metric_test.go Adds comprehensive test coverage for new JSON and pflag functionality


// Set implements the pflag.Value interface.
func (s *ValidationScheme) Set(text string) error {
switch text {
case "":
// Don't change the value.
Copy link
Preview

Copilot AI Jul 31, 2025

Choose a reason for hiding this comment

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

The comment 'Don't change the value' is misleading for an empty string input. When Set() is called with an empty string, it should set the value to UnsetValidation, not leave it unchanged. Either update the logic to set *s = UnsetValidation or clarify the comment to explain why empty strings preserve the current value.

Suggested change
// Don't change the value.
// Set to UnsetValidation for empty string input.
*s = UnsetValidation

Copilot uses AI. Check for mistakes.

@juliusmh juliusmh force-pushed the jmh/validation_scheme_pflag_json branch from 06cb330 to 8454829 Compare July 31, 2025 12:00
@aknuds1 aknuds1 dismissed their stale review July 31, 2025 12:02

Stale.

@aknuds1 aknuds1 self-requested a review July 31, 2025 12:02
…er/Unmarshaler interfaces

Signed-off-by: Julius Hinze <[email protected]>
@juliusmh juliusmh force-pushed the jmh/validation_scheme_pflag_json branch from 8454829 to 2e9eb8f Compare August 1, 2025 08:18
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.

2 participants