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

Refactor Recorder and improve API #3

Open
codemental opened this issue Jan 19, 2024 · 0 comments
Open

Refactor Recorder and improve API #3

codemental opened this issue Jan 19, 2024 · 0 comments
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@codemental
Copy link
Member

codemental commented Jan 19, 2024

The Recorder does not offer a clean API. Inside the Comparator we have calls like this:
AppendValidationErrorSignal("number of fields does not match") where the message is defined as a method parameter, and then we have methods like this inside the DefaultRecorder itself:
recorder.logResult.WriteString(fmt.Sprintf("%s <-- ignoring field\n", indent)).

It would be better for all methods to receive the jsonPath, fieldName and fieldValue from the comparator, and all messages (text) that are used to create the recorder log should be defined inside the Recorder implementation itself (in our case, the DefaultRecorder). This will offer a much cleaner way for other Recorder implementations to change the way the output looks and offer them full control.

Also, the Recorder uses fmt.Sprintf(...) internally in some cases which should not happen at all, since we are using a strings.Builder (!)

Action Points

  • refactor API as mentioned above (I imagine we will have specific methods for different validation cases)
  • remove the fmt.Sprintf(...) completely and only use the strings.Builder API
  • write a dedicated test for the DefaultRecorder (currently it is tested inside the Comparator tests)
@codemental codemental added bug Something isn't working good first issue Good for newcomers labels Jan 19, 2024
@codemental codemental changed the title Recorder API Refactor Recorder and improve API Jan 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

1 participant