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

Add arc and solid_arc method to FPDF #266

Merged
merged 4 commits into from
Oct 26, 2021

Conversation

tabarnhack
Copy link

According to #258, this PR adds the 2 methods arc and solid_arc.

arc and solid_arc methods allow to draw respectively an arc and a solid arc in a PDF file in a similar way to the rect, circle and ellipse methods.

Tests and documentation have also been improved in order to add these 2 features.

arc and solid_arc methods allow to draw respectively an arc and a solid
arc in a PDF file in a similar way to the rect, circle and ellipse
methods.
@tabarnhack
Copy link
Author

I'm not sure where the test failed. Can you provide me with some details so that I can correct my mistake please ?

@Lucas-C
Copy link
Member

Lucas-C commented Oct 25, 2021

Sure! And thanks for your contribution!

Seems like pylint is complaining because of the high similarities of the functions test_arc_line_width & test_solid_arc_line_width.
Feel free to add a # pylint: disable=duplicate-code on the definition line of those 2 functions.
That should get rid of this warning.

A side question: are you doing this for hacktoberfest?
Just to know if you're aiming to have this merged before the end of the month 😉

@tabarnhack
Copy link
Author

Great ! Thanks for the tip I'm not familiar with pylint. I will make the changes to disable the errors as, even if the 2 functions are similar, they don't really have the same purposes.

Indeed, I'm participating to Hacktoberfest despite the late timing 😄

@tabarnhack
Copy link
Author

Unfortunately, pylint seems to have an issue about disabling the duplicate-code rule for a piece of code as stated in the issue from the pylint project. I've tried several things but the error is triggered everytime.

@Lucas-C
Copy link
Member

Lucas-C commented Oct 25, 2021

Damn.

A few ideas:

  • try to place the comment on the very first line of both files
  • try to slightly change the code of one of the function to avoid triggering the check
  • try to create a test/.pylintrc that disables this check

If everything fails, I'm fine with disabling this check in the root .pylintrc
if it causes more annoying false positives than relevant warnings.

@Lucas-C
Copy link
Member

Lucas-C commented Oct 25, 2021

@allcontributors please add @tabarnhack for code

@allcontributors
Copy link

@Lucas-C

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

@tabarnhack
Copy link
Author

Okay, I've found a workaround even if I'm not really proud of it 😅

@Lucas-C
Copy link
Member

Lucas-C commented Oct 26, 2021

Nice!
You just need to-generate the reference PDF for test_solid_arc_line_width,
as it is now failing. Just passing generate=True once to assert_pdf_equal in this function should do the trick.

There was an error in this test which make it fail everytime due to
differences between how the 2 compared files are built
@codecov
Copy link

codecov bot commented Oct 26, 2021

Codecov Report

Merging #266 (2df33ed) into master (75770cb) will increase coverage by 0.04%.
The diff coverage is 87.87%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #266      +/-   ##
==========================================
+ Coverage   87.05%   87.09%   +0.04%     
==========================================
  Files          16       16              
  Lines        3753     3819      +66     
  Branches      805      814       +9     
==========================================
+ Hits         3267     3326      +59     
- Misses        315      319       +4     
- Partials      171      174       +3     
Impacted Files Coverage Δ
fpdf/fpdf.py 83.86% <87.87%> (+0.20%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 75770cb...2df33ed. Read the comment docs.

Copy link
Member

@Lucas-C Lucas-C left a comment

Choose a reason for hiding this comment

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

You did an excellent job here!
The code, the doc and the tests are great.
I suggested some minor changes.
Could you also please a short mention of your addition to CHANGELOG.md?

fpdf/fpdf.py Outdated Show resolved Hide resolved
fpdf/fpdf.py Outdated Show resolved Hide resolved
fpdf/fpdf.py Show resolved Hide resolved
fpdf/fpdf.py Show resolved Hide resolved
fpdf/fpdf.py Outdated Show resolved Hide resolved
test/shapes/test_arc.py Outdated Show resolved Hide resolved
Several changes according to reviews from the PR. Changes are:
- More testing cases for arc and solid_arc
- Typo correction
- Style value handling
- Unnecessary pylint comment removed
@Lucas-C Lucas-C merged commit adbe35e into py-pdf:master Oct 26, 2021
@Lucas-C
Copy link
Member

Lucas-C commented Oct 26, 2021

Perfect!

I merged your PR, thank you for your contribution!

@Lucas-C
Copy link
Member

Lucas-C commented Nov 19, 2021

FYI I mentioned you in this blog article 😉
https://chezsoi.org/lucas/blog/hacktoberfest-on-fpdf2.html

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

Successfully merging this pull request may close these issues.

2 participants