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

minor adjustment to image testing to support png #152

Merged
merged 5 commits into from
Dec 16, 2024

Conversation

ludomitch
Copy link
Collaborator

No description provided.

@dosubot dosubot bot added size:S This PR changes 10-29 lines, ignoring generated files. enhancement New feature or request labels Dec 16, 2024
Copy link
Collaborator

@maykcaldas maykcaldas left a comment

Choose a reason for hiding this comment

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

It seems a pre-commit failed because there are commits with different email/username. You can update .mailmap to address that.

Check: https://git-scm.com/docs/gitmailmap

Can you add something like:
Proper Name <proper@email.xx> Commit Name <commit@email.xx>

to the .mailmap file?

EDIT: The error says

The following email addresses are associated with more than one name:

        ludo@futurehouse.org

The associations include:

      1 Ludovico Mitchener <ludo@futurehouse.org>
      1 ludomitch <ludo@futurehouse.org>

Can you try adding:

Ludovico Mitchener <ludo@futurehouse.org> ludomitch

to see if it will solve the issue?

@ludomitch ludomitch requested a review from maykcaldas December 16, 2024 06:49
.mailmap Outdated Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't the other image stub in this repo basically the same name? Can we give them more verbose names to differentiate between the two easier

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The content of the image isn't as important as the file type. The reason I came across this bug is bc the b64.decode method doesn't treat png prefixed and jpeg prefixed b64 images in the same way so better to test for both. Are you ok if I rename them to sample_jpeg_image.b64 and sample_png_image.b64, respectively?

Copy link
Collaborator

@maykcaldas maykcaldas left a comment

Choose a reason for hiding this comment

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

LGTM. Just suggested a minor name change

@@ -73,7 +73,9 @@ def encode_image_to_base64(img: "np.ndarray") -> str:
def check_if_valid_base64(image: str) -> str:
Copy link
Collaborator

Choose a reason for hiding this comment

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

With this name, I'd expect this function to return a boolean.
Can you change the name to something like validate_base64?
Also, can you make sure the docstring says it returns the image in case it is a valid base64?

@ludomitch ludomitch merged commit 4132491 into main Dec 16, 2024
6 checks passed
@ludomitch ludomitch deleted the support-png-base64-images branch December 16, 2024 23:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request size:S This PR changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants