Skip to content

Update naming of filter containers function && container list handler#2067

Merged
Zheaoli merged 1 commit into
containerd:mainfrom
suyanhanx:container-list-filterFunc-naming
Mar 2, 2023
Merged

Update naming of filter containers function && container list handler#2067
Zheaoli merged 1 commit into
containerd:mainfrom
suyanhanx:container-list-filterFunc-naming

Conversation

@suyanhanx
Copy link
Copy Markdown
Contributor

Update the naming of the containers filter function and container list handler to match the current naming pattern.

Signed-off-by: Han Xu suyanhanx@gmail.com

Copy link
Copy Markdown
Member

@Zheaoli Zheaoli left a comment

Choose a reason for hiding this comment

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

LGTM

@Zheaoli
Copy link
Copy Markdown
Member

Zheaoli commented Mar 2, 2023

This PR is a little bit different with #2041. I think we should keep one style in command style. I prefer #2067

@suyanhanx
Copy link
Copy Markdown
Contributor Author

I forgot to review it. Otherwise, I would have commented in that PR.

@Zheaoli Zheaoli merged commit d7cf99e into containerd:main Mar 2, 2023
@Zheaoli Zheaoli added this to the v1.3.0 milestone Mar 2, 2023
@suyanhanx suyanhanx deleted the container-list-filterFunc-naming branch March 2, 2023 13:02
@davidhsingyuchen
Copy link
Copy Markdown
Contributor

@Zheaoli Thanks for merging #2041.

This PR is a little bit different with #2041. I think we should keep one style in command style. I prefer #2067

I agree that ideally there should be only command style, but #2067 (i.e., this PR) unexports the "main logic" function (see #1855 (comment) for more details), which means that other projects cannot reuse that function.

I went with ListCommandHandler in #2041 because #1923 had been merged, and I'm willing to do a one-shot PR to change all the commands to use that style, but if that style is not preferred, could you help shed some light on if I can just go ahead and change filterContainers to be FilterContainers?

As a side note, I feel that filterContainers sounds that the input of it is already a list of containers, and what it does is only filters out some of them, while it's not the case, so the semantics of it could be a bit off. Don't have a strong opinion on this though.

@suyanhanx
Copy link
Copy Markdown
Contributor Author

but #2067 (i.e., this PR) unexports the "main logic" function (see #1855 (comment) for more details), which means that other projects cannot reuse that function.

This is a minor issue. I think we can change it when needed.

As a side note, I feel that filterContainers sounds that the input of it is already a list of containers, and what it does is only filters out some of them, while it's not the case, so the semantics of it could be a bit off. Don't have a strong opinion on this though.

Not without reason. If you come up with other good names, you can also propose them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants