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

feat: add image validation #982

Merged
merged 8 commits into from
May 21, 2023

Conversation

the-fanan
Copy link
Contributor

@the-fanan the-fanan commented Jul 30, 2022

Enhances

Make sure that you've checked the boxes below before you submit PR:
☑️ Tests exist or have been written that cover this particular change.

This PR does the following:

  • adds a baked in validation to validate a file is a valid image
  • adds a unit test to cover the added validation
  • adds translations for image validation
  • adds tests to cover translations for image validation
  • updates the README.md document to include the added image validation

Motivation:

  • Go is increasingly becoming used as a back-end language. File validations are pretty common for many back-end services. This validation will allow developers to easily validate that a file is a valid image.

@go-playground/validator-maintainers

@the-fanan the-fanan requested a review from a team as a code owner July 30, 2022 08:44
@coveralls
Copy link

coveralls commented Jul 30, 2022

Coverage Status

Coverage: 73.945% (+0.07%) from 73.88% when pulling e3ad39e on the-fanan:feat-add-image-validation into c8b98cb on go-playground:master.

@the-fanan the-fanan closed this Jul 30, 2022
@the-fanan the-fanan reopened this Jul 30, 2022
@the-fanan
Copy link
Contributor Author

@hjwk can you help review this please?

Copy link

@hjwk hjwk left a comment

Choose a reason for hiding this comment

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

Sure, can do a quick review but I am not an owner...
Don't have much to say, but please check code alignment (I've highlighted one example in the review)

translations/ja/ja_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@deankarn deankarn left a comment

Choose a reason for hiding this comment

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

A few changes and I think this PR would be good to go.

baked_in.go Outdated Show resolved Hide resolved
baked_in.go Outdated
Comment on lines 1463 to 1466
_, err = file.Seek(0, io.SeekStart)
if err != nil {
panic(fmt.Sprintf("Could not reset seek start for file. Error: %s", err))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure why the seek is required? should be removed from what I can tell.

Copy link
Contributor Author

@the-fanan the-fanan Mar 24, 2023

Choose a reason for hiding this comment

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

While reading data on the mime type of the file, the offset for the reader is moved away from the beginning of the file and this may cause issues for other parts of the program that need to read from that file. Using seek ensures that after the mime type is read, the offset of the reader is returned to the beginning of the file so that there will be no issues for other parts of the program that will need to read from that file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right I understand what Seek does and why one would need to use it but in this case the file handle is closed when this function ends because of the defer file.Close() and nothing else tries to read using that handle and so there is no need to Seek from what I can tell.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, got you.

translations/ar/ar_test.go Show resolved Hide resolved
@the-fanan the-fanan requested a review from deankarn March 24, 2023 21:39
@hiendaovinh
Copy link

Let's ship this please 👍

@the-fanan the-fanan requested a review from hjwk April 4, 2023 16:39
@deankarn deankarn merged commit 5ac1643 into go-playground:master May 21, 2023
6 checks passed
@@ -1321,6 +1321,11 @@ func RegisterDefaultTranslations(v *validator.Validate, trans ut.Translator) (er
translation: "{0} deve ser um valor booleano válido",
override: false,
},
{
tag: "image",
translation: "{0} deve ser uma imagen válido",

Choose a reason for hiding this comment

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

The correct translation is
"{0} deve ser uma imagem válida"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants