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

Added PDF, PNG, JPEG convenience body types #65

Merged
merged 6 commits into from
Oct 17, 2024
Merged

Added PDF, PNG, JPEG convenience body types #65

merged 6 commits into from
Oct 17, 2024

Conversation

toastx
Copy link
Contributor

@toastx toastx commented Oct 17, 2024

Expanded Body of parser/src/schema/body.rs for body.pdf, body.png, body.jpeg as per #63

2024-10-17.12-43-44.mp4

@kriogenia kriogenia self-requested a review October 17, 2024 09:31
@kriogenia kriogenia added trigger Use to trigger actions and removed trigger Use to trigger actions labels Oct 17, 2024
Copy link
Owner

@kriogenia kriogenia left a comment

Choose a reason for hiding this comment

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

Hi again, @toastx and thank you so much for taking time to make this awesome contribution. I made a few comments regarding some little details that you probably forgot to fix or restore. The main logic and tests are perfect, so if you fix that I'll merge asap.

If you can't apply those fixes, don't sweat it. Just tell me and I'll do it myself. Thanks again.

bin/example.toml Outdated
Copy link
Owner

Choose a reason for hiding this comment

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

remove this file, i gues that it's a leftover for some test that you did

Content-Type = "image/jpeg"

[body]
file = "D:/open-source/rede/bin/tests/assets/earth.jpeg" #modify relative path
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
file = "D:/open-source/rede/bin/tests/assets/earth.jpeg" #modify relative path
file = "./tests/assets/earth.jpeg"

Content-Type = "application/pdf"

[body]
file = "D:/open-source/rede/bin/tests/assets/dummy-pdf_2.pdf" #modify relative path
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
file = "D:/open-source/rede/bin/tests/assets/dummy-pdf_2.pdf" #modify relative path
file = "./tests/assets/dummy-pdf_2.pdf"

Content-Type = "image/png"

[body]
file = "D:/open-source/rede/bin/tests/assets/moon.png" #modify relative path
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
file = "D:/open-source/rede/bin/tests/assets/moon.png" #modify relative path
file = "./tests/assets/moon.png"

Pdf(String),
#[serde(alias = "png")]
Png(String),
#[serde(alias = "jpeg")]
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
#[serde(alias = "jpeg")]
#[serde(alias = "jpg", "jpeg")]

Let's also support jpg as a way to invoke

bin/Cargo.toml Outdated
Comment on lines 35 to 41
# rede_parser = "0.2.2"
# rede_placeholders = "0.1.0"
# rede_schema = "0.2.0"

# rede_parser = { path = "../parser" } # local
# rede_placeholders = { path = "../placeholders"} # local
# rede_schema = { path = "../schema" } # local
rede_parser = { path = "../parser" } # local
rede_placeholders = { path = "../placeholders" } # local
rede_schema = { path = "../schema" } # local
Copy link
Owner

Choose a reason for hiding this comment

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

Please, restore it back to its previous state

@kriogenia kriogenia added feature New feature parser Changes in the parser crate hacktoberfest labels Oct 17, 2024
@toastx
Copy link
Contributor Author

toastx commented Oct 17, 2024

My bad, i forgot to remove the local dependencies. Also , giving path like that isnt working... it prompts rede to be in the root
Ill change it as per your request tho..

@kriogenia
Copy link
Owner

kriogenia commented Oct 17, 2024

My bad, i forgot to remove the local dependencies. Also , giving path like that isnt working... it prompts rede to be in the root Ill change it as per your request tho..

Don't worry about the request not working right now. It will work once I set-up an integration test when we upgrade the parser version as the tests executed from the bin root (you can see in the body_binary.toml that is set-up the same way).

Merging!

@kriogenia kriogenia merged commit 66b13d2 into kriogenia:main Oct 17, 2024
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants