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 Metadata.toXContent by extracting methods #124689

Conversation

ywangd
Copy link
Member

@ywangd ywangd commented Mar 13, 2025

The method is long and has two distinct paths for multi-project and single-project formats. This PR extracts separate method for each of the code paths for readability.

See also:
#124613 (comment)

The method is long and has two distinct paths for multi-project and
single-project formats. This PR extracts separate method for each of the
code paths for readability.

See also:
elastic#124613 (comment)
@ywangd ywangd added :Core/Infra/Core Core issues without another label >refactoring v9.1.0 labels Mar 13, 2025
@ywangd ywangd requested review from pxsalehi and nielsbauman March 13, 2025 01:24
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@elasticsearchmachine elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Mar 13, 2025
Copy link
Contributor

@nielsbauman nielsbauman left a comment

Choose a reason for hiding this comment

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

LGTM, thanks Yang!


// need to combine reserved state together into a single block so we don't get duplicate keys
// and not include it in the project xcontent output (through the lack of multi-project params)
// use a tree map so the order is deterministic
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need the tree map in the MP version?

Copy link
Member Author

Choose a reason for hiding this comment

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

I actually have no idea. This is existing code. I thought the order was relied on tests. But I could not find any evidence.

@ywangd
Copy link
Member Author

ywangd commented Mar 13, 2025

@elasticmachine update branch

@ywangd ywangd added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Mar 13, 2025
@elasticsearchmachine elasticsearchmachine merged commit cd25958 into elastic:main Mar 13, 2025
17 checks passed
@ywangd ywangd deleted the extract-methods-for-metadata-toxcontent branch March 13, 2025 13:31
jfreden pushed a commit to jfreden/elasticsearch that referenced this pull request Mar 13, 2025
The method is long and has two distinct paths for multi-project and
single-project formats. This PR extracts separate method for each of the
code paths for readability.

See also:
elastic#124613 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) :Core/Infra/Core Core issues without another label >refactoring Team:Core/Infra Meta label for core/infra team v9.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants