Skip to content

Conversation

@nastra
Copy link
Contributor

@nastra nastra commented Sep 20, 2022

While reviewing #5799 and the StatisticsFileParser I noticed that there are a bunch of places with duplicated code in the JSON Parsers. This PR aims at cleaning those up and reusing JsonUtil.generate where applicable

@nastra nastra force-pushed the reduce-code-duplication-in-parsers branch from 29176f6 to 9bf8250 Compare September 20, 2022 08:17
} catch (IOException e) {
throw new RuntimeIOException(e);
}
return JsonUtil.generate(gen -> toJson(spec, gen), pretty);
Copy link
Member

Choose a reason for hiding this comment

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

Nice!

BTW we have a bit duplicated code for parsing as well. I was changing it in #5048.

BTW 2: i'd use generator instead of gen. That's how it's usually called, so it will be more familiar

Copy link
Member

Choose a reason for hiding this comment

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

i'd use generator instead of gen.

i see now both are used in the codebase. disregard

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BTW we have a bit duplicated code for parsing as well. I was changing it in #5048.

ah interesting, you're right. Do you want to open maybe a separate PR for removing that part of duplicated code?

@github-actions github-actions bot added the core label Sep 20, 2022
Copy link
Contributor

@danielcweeks danielcweeks left a comment

Choose a reason for hiding this comment

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

Thanks @nastra, nice cleanup

@Fokko Fokko merged commit a2c5380 into apache:master Sep 21, 2022
@nastra nastra deleted the reduce-code-duplication-in-parsers branch June 1, 2023 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants