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

fix: Outdated image string join logic in ImageRow.image_ref #3125

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

jopemachine
Copy link
Member

@jopemachine jopemachine commented Nov 21, 2024

Fixes #3126, follow-up to #2978.

Since project value was changed to be nullable in PR #2978, the image string join logic also should be updated.

Why?

For example, if the registry is "abc" and the project is "", then f"{self.registry}/{self.project}/" becomes "abc//". (Expected result: "abc/")


Checklist: (if applicable)

  • Milestone metadata specifying the target backport version

@jopemachine jopemachine added the type:bug Reports about that are not working label Nov 21, 2024
@jopemachine jopemachine added this to the 24.09 milestone Nov 21, 2024
@jopemachine jopemachine self-assigned this Nov 21, 2024
@github-actions github-actions bot added comp:manager Related to Manager component size:XS ~10 LoC labels Nov 21, 2024
@jopemachine jopemachine changed the title fix: Outdated image string join logic fix: Outdated image string join logic in ImageRow.image_ref Nov 21, 2024
@jopemachine jopemachine marked this pull request as ready for review November 21, 2024 23:01
@jopemachine jopemachine linked an issue Nov 21, 2024 that may be closed by this pull request
Copy link
Member

@fregataa fregataa left a comment

Choose a reason for hiding this comment

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

Good. After this PR, let's refactor this codes by replacing split() to partition() and adding type hints to the join_non_empty() function

@jopemachine
Copy link
Member Author

Good. After this PR, let's refactor this codes by replacing split() to partition() and adding type hints to the join_non_empty() function

@fregataa You might be referring to the following code.
By the way, is there a specific reason we prefer partition() over split()?
I don't see significant readability difference between code that uses split() and code that uses partition().

image_and_tag = self.name.partition(f"{join(self.registry, self.project)}/")[2]

@fregataa
Copy link
Member

@fregataa You might be referring to the following code. By the way, is there a specific reason we prefer partition() over split()? I don't see significant readability difference between code that uses split() and code that uses partition().

image_and_tag = self.name.partition(f"{join(self.registry, self.project)}/")[2]

partition() always returns a tuple of three strings but split() returns a list of strings.
you can use split() if you expect the string value will be divided into more than 2 parts. but if you are sure that the value is divided into 2 parts, using partition() makes it easy to understand how the result comes from the original value.

image_and_tag = self.name.partition(f"{join(self.registry, self.project)}/")[2] # This is okay but
_, _, image_and_tag = self.name.partition(f"{join(self.registry, self.project)}/") # I recommend to use this pattern with partition()

@jopemachine
Copy link
Member Author

@fregataa You might be referring to the following code. By the way, is there a specific reason we prefer partition() over split()? I don't see significant readability difference between code that uses split() and code that uses partition().

image_and_tag = self.name.partition(f"{join(self.registry, self.project)}/")[2]

partition() always returns a tuple of three strings but split() returns a list of strings. you can use split() if you expect the string value will be divided into more than 2 parts. but if you are sure that the value is divided into 2 parts, using partition() makes it easy to understand how the result comes from the original value.

image_and_tag = self.name.partition(f"{join(self.registry, self.project)}/")[2] # This is okay but
_, _, image_and_tag = self.name.partition(f"{join(self.registry, self.project)}/") # I recommend to use this pattern with partition()

Thanks for your description! I will reflect this.

Copy link

@HyeockJinKim HyeockJinKim left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:manager Related to Manager component size:XS ~10 LoC type:bug Reports about that are not working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Image string join logic in ImageRow.image_ref is outdated
3 participants