Skip to content

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