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

Improve Display Impl - Each ValidationError should be on its own line #235

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

Conversation

aarashy
Copy link

@aarashy aarashy commented Oct 12, 2022

The current impl for Display doesn't break newlines frequently enough.

The display output of the list variant of ValidationErrorsKind currently looks like the following:

my_field[0].subfield: Validation error: Some error. [{"value": String("some-value")}]my_field[1].subfield: Validation error: Some error. [{"value": String("some-value-2")}]
other_field[0].other: Validation error: Other error [{"value": String("other-value")}]

It only breaks line when the top-level field of the ValidationErrors changes. I would prefer for it to look like the following:

my_field[0].subfield: Validation error: Some error. [{"value": String("some-value")}]
my_field[1].subfield: Validation error: Some error. [{"value": String("some-value-2")}]

other_field[0].other: Validation error: Other error [{"value": String("other-value")}]

It seems pretty clear that separate field-level errors belong on their own line - having multiple on the same line always looks bad. Having multiple line-breaks when the top-level field changes doesn't feel like a regression either, but we could remove the existing writeln! call if we wanted a maximum of 1 new line after every error display.

@aarashy aarashy changed the title Improve Display Impl - Each ValidatorError should be on its own line Improve Display Impl - Each ValidationError should be on its own line Oct 12, 2022
@aarashy
Copy link
Author

aarashy commented Oct 20, 2022

Hi, thanks for the approval - the issue with CI seems unrelated to this PR, right?

@Keats
Copy link
Owner

Keats commented Oct 21, 2022

It seems unrelated yes

@aarashy
Copy link
Author

aarashy commented Oct 24, 2022

Would you like me to re-run the CI or will you merge this later?

@Keats
Copy link
Owner

Keats commented Oct 25, 2022

That's fine to leave like this, I'll merge for the next release

@dslemusp
Copy link

is this going to be merged?

@Keats
Copy link
Owner

Keats commented Jul 21, 2024

The concept probably, but ideally there will be an error rewrite for the next version to make it simpler.

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