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 FastAPI documentation #760

Merged
merged 7 commits into from
Apr 12, 2023
Merged

Conversation

KamarulAdha
Copy link

@KamarulAdha KamarulAdha commented Apr 10, 2023

Added documentation on how to use FPDF using FastAPI and send the file via SMTP.

Checklist:

  • The GitHub pipeline is OK (green),
    meaning that both pylint (static code analyzer) and black (code formatter) are happy with the changes of this PR.

  • A unit test is covering the code added / modified by this PR

  • This PR is ready to be merged

  • In case of a new feature, docstrings have been added, with also some documentation in the docs/ folder

  • A mention of the change is present in CHANGELOG.md

By submitting this pull request, I confirm that my contribution is made under the terms of the GNU LGPL 3.0 license.

@Lucas-C
Copy link
Member

Lucas-C commented Apr 10, 2023

Hi and welcome @KamarulAdha!

Thank you for contriburing to fpdf2 😊

Your document section is excellent! Clear, consise, well structured. Good job!

Could you also please add a mention of your addition in the CHANGELOG.md file, as part of this PR, please?

@Lucas-C
Copy link
Member

Lucas-C commented Apr 10, 2023

@allcontributors please add @KamarulAdha for documentation

@allcontributors
Copy link

@Lucas-C

I've put up a pull request to add @KamarulAdha! 🎉

docs/UsageInWebAPI.md Outdated Show resolved Hide resolved
docs/UsageInWebAPI.md Outdated Show resolved Hide resolved
docs/UsageInWebAPI.md Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Apr 10, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (2a0b280) 92.67% compared to head (6d60817) 92.67%.

❗ Current head 6d60817 differs from pull request most recent head 74898b8. Consider uploading reports for the commit 74898b8 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #760   +/-   ##
=======================================
  Coverage   92.67%   92.67%           
=======================================
  Files          27       27           
  Lines        7275     7275           
  Branches     1317     1317           
=======================================
  Hits         6742     6742           
  Misses        337      337           
  Partials      196      196           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@KamarulAdha
Copy link
Author

You mentioned that I need to add my name in the CHANGELOG.md file. Do I make a new commit and push it on to this branch? Or should I fork the main repo, make a new branch, then commit the new changes?

@Lucas-C
Copy link
Member

Lucas-C commented Apr 11, 2023

You mentioned that I need to add my name in the CHANGELOG.md file. Do I make a new commit and push it on to this branch? Or should I fork the main repo, make a new branch, then commit the new changes?

Yes, you can simply add extra commits to this branch, with the changes adressing my review comments, and a new line like this in CHANGELOG.md:

### Added
- documentation on how to use `fpdf2` with [FastAPI](https://fastapi.tiangolo.com/): <https://pyfpdf.github.io/fpdf2/UsageInWebAPI.html#FastAPI> thanks to @KamarulAdha

@KamarulAdha
Copy link
Author

I've changed the bytesIO and the str().

I tried using aiosmtplib instead of smtplib and there were complications. Not sure if the problem was coming from the smtp server provider or the code itself. But I didn't want to waste any of your time, so I made a PR with only those 2 changes for now. Alongside with the CHANGELOG.md.

I do think, since I wasn't able to use aiosmtplib, I could make the entire endpoint non-async. That way, everything is more standardized. What do you think? Should I try again with aiosmtplib, or remove async from the endpoint entirely?

@Lucas-C
Copy link
Member

Lucas-C commented Apr 11, 2023

I do think, since I wasn't able to use aiosmtplib, I could make the entire endpoint non-async. That way, everything is more standardized. What do you think? Should I try again with aiosmtplib, or remove async from the endpoint entirely?

Yes, I agree, a synchronous function would make more sense if aiosmtplib is not used 😊

You will still be able to modify the code in another PR later on, if you figure out how to use aiosmtplib in that context!

@KamarulAdha
Copy link
Author

It turns out, FastAPI has a alot of trouble with non-async endpoints receiving json objects as input. I managed to remove the async endpoint, but as a result, I had to add two more functions (utility and helper functions).

This would allow the endpoint to receive and process the json object. Aside from that, I added try-except in the endpoint just in case anything breaks. And also some explanation for each of the functions so that it is a bit clearer for the reader.

Many thanks~!

@Lucas-C
Copy link
Member

Lucas-C commented Apr 12, 2023

It turns out, FastAPI has a alot of trouble with non-async endpoints receiving json objects as input. I managed to remove the async endpoint, but as a result, I had to add two more functions (utility and helper functions).

OK, I understand.

Given it adds a bit of complexity to the example, and the fact that this code is not really synchronous as run_in_thread() is used, I have to say that I prefer the original async version in terms of brevity and clarity.

What do you think?
Would you be OK to get back to your original, shorter async function?

@KamarulAdha
Copy link
Author

It turns out, FastAPI has a alot of trouble with non-async endpoints receiving json objects as input. I managed to remove the async endpoint, but as a result, I had to add two more functions (utility and helper functions).

OK, I understand.

Given it adds a bit of complexity to the example, and the fact that this code is not really synchronous as run_in_thread() is used, I have to say that I prefer the original async version in terms of brevity and clarity.

What do you think? Would you be OK to get back to your original, shorter async function?

I am okay with that. Also, I would like to mention. I tried something new and I removed the sending email functionality. Instead, I made the API to return the file back to the user. So, the user sends data to the API, the API returns back the PDF back.

This way, it is more relatable with the other guides. With this, we are making the endpoint async, but we are not sending my email anymore as I had problems with aiosmtp.

What do you think?

Option 1:
Make the endpoint async but keeping the email send function.

Option 2:
Make the endpoint async, but send the file back as a response.

@Lucas-C
Copy link
Member

Lucas-C commented Apr 12, 2023

Instead, I made the API to return the file back to the user. So, the user sends data to the API, the API returns back the PDF back.

This way, it is more relatable with the other guides. With this, we are making the endpoint async, but we are not sending my email anymore as I had problems with aiosmtp.

What do you think?

Sounds great!
Even better in terms of pedagogy, good thinking 😊
Let's go with option 2!

@KamarulAdha
Copy link
Author

KamarulAdha commented Apr 12, 2023

Alright, I think I've made the necessary changes. Do let me know if there are any problems with it. Many thanks~!

EDIT: I forgot to change the explanation at the top that explains what the code does. Submitting new PR now.

docs/UsageInWebAPI.md Outdated Show resolved Hide resolved
docs/UsageInWebAPI.md Outdated Show resolved Hide resolved
@Lucas-C Lucas-C merged commit a3cf95d into py-pdf:master Apr 12, 2023
@Lucas-C
Copy link
Member

Lucas-C commented Apr 12, 2023

Merged!

Thank you for your contribution @KamarulAdha 👍

@Lucas-C Lucas-C changed the title Added FastAPI documentation. Send PDF via SMTP Added FastAPI documentation Apr 12, 2023
@Lucas-C
Copy link
Member

Lucas-C commented Apr 12, 2023

Your contribution is now online & public:
https://pyfpdf.github.io/fpdf2/UsageInWebAPI.html#fastapi

@KamarulAdha
Copy link
Author

Your contribution is now online & public: https://pyfpdf.github.io/fpdf2/UsageInWebAPI.html#fastapi

Thank you so much for taking the time to teach me on how to contribute on GitHub. This was an exciting experience. Thank you~!

Also, thanks for renaming the PR. Looks even more professional now ^^
Cheers~

@Lucas-C
Copy link
Member

Lucas-C commented Apr 13, 2023

My pleasure 😊

You have also been added to the list of contributors:
https://github.com/PyFPDF/fpdf2#contributors-

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.

2 participants