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

Issue-143: Implement Pretty Printing #173

Closed
wants to merge 3 commits into from

Conversation

manstis
Copy link

@manstis manstis commented Apr 19, 2022

See #143

@manstis
Copy link
Author

manstis commented May 11, 2022

Hi @SlyngDK IDK could you please approve running of the workflows?

This would make use of your extension easier in development environments where a formatted log output is handy.

Thank-you.

@manstis
Copy link
Author

manstis commented Sep 12, 2022

Hi @SlyngDK is this PR of any interest?

1 similar comment
@manstis
Copy link
Author

manstis commented Oct 24, 2022

Hi @SlyngDK is this PR of any interest?

@manstis
Copy link
Author

manstis commented Jan 5, 2023

@gastaldi @gsmet IDK if this PR is of any interest?

I don't get much (erm, any) reply from @SlyngDK but I see you've both been involved with this repository over time

(and you're RH!)

@SlyngDK
Copy link
Contributor

SlyngDK commented Jan 5, 2023

I remember i tested, but was not formatted pretty as expected, maybe only one of the formatters. Also the testing not check the pretty formatting is done.

@manstis
Copy link
Author

manstis commented Jan 5, 2023

Hi @SlyngDK

not formatted pretty as expected

"pretty print" is only supported for JSON

the testing not check the pretty formatting is done

The tests do check the if "pretty formatting" is done. See the change here.

See PrettyPrintingJsonProviderJsonbTest and PrettyPrintingJsonProviderJacksonTest.

@manstis
Copy link
Author

manstis commented Jan 5, 2023

As an example.

Test (the most complex in the suite)
assertEquals(format("{\"key\":{\"field1\":\"field1\",\"field2\":2389472389}}"), run("key", new TestPojo()));

KeyValueStructuredArgumentJsonbTest.testKeyValues() returns:

{"key":{"field1":"field1","field2":2389472389}}

PrettyPrintingJsonProviderJsonbTest.testKeyValues() returns:

{
    "key": {
        "field1": "field1",
        "field2": 2389472389
    }
}

private static final JsonFactory jsonb = new JsonbJsonFactory();
private static final JsonFactory jackson = new JacksonJsonFactory();
private static final ObjectMapper mapper = new ObjectMapper();
private final JsonFactory jsonb = new JsonbJsonFactory(prettyPrint());
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like the way the test is created. And the test should not format the output, because what are you then running your test against, not the output of the actual JsonFormatter.

Copy link
Author

@manstis manstis Jan 6, 2023

Choose a reason for hiding this comment

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

OK, fair enough.

I had tried to keep duplication to a minimum.

The JsonGenerator in quarkus-logging-json delegates "pretty printing" to their underlying JSON library and as such the format for the pretty printed JSON differs between the two (line breaks, indenting and spacing). I added the format(..) method (and related code) around the expected value so that the test could format the expected value with the underlying JSON library thus avoiding duplicating the literal values for the different JSON libraries.

I've added a new commit removing that and duplicating the literal expected values for each JsonGenerator.

This is your project and I respect that. I missed being able to "pretty print" when using it so thought I'd try adding the missing implementation. I'm happy to adhere to your requirements, guidelines etc. I think having the missing implementation remains a useful addition.

Copy link
Author

Choose a reason for hiding this comment

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

@SlyngDK Would you like me to make any further changes?

@manstis
Copy link
Author

manstis commented Jul 27, 2023

Closing.

It seems this will never be merged.

@manstis manstis closed this Jul 27, 2023
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.

None yet

2 participants