-
Notifications
You must be signed in to change notification settings - Fork 254
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 Cell level control for Table Borders #1285
Conversation
Hi @Lucas-C, I have made a PR to know if I am doing it correctly it is not a complete implementation, please see the code change and give me suggestions. |
Nice, thank you for creating a PR 🙂 I made a few comments. One thing that should come next is to add unit tests! 🙂 |
Hi, I made the suggested changes, now how can I fix this pylint warning, And please tell me where I should add the unit tests. |
It now seems to be passing, but you have however 27 failing tests.
You can add them to |
Hi @Lucas-C, should I write test just like this? and for all the different values like, def test_cell_border_left(tmp_path):
pdf = FPDF()
pdf.add_page()
pdf.set_font("Times", size=5)
with pdf.table( gutter_height=2, gutter_width=2) as table:
for data_row in TABLE_DATA:
row = table.row()
for datum in data_row:
row.cell(datum,border="left")
assert_pdf_equal(pdf, HERE / "test_cell_border_left.pdf", tmp_path) |
@Lucas-C is it the right way of writing the tests? |
Yes, this look likes a good start 👍 And testing many possible combination of values is also a good idea! |
is it fine @Lucas-C ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it fine @Lucas-C ?
There are a couple of minor things remaining to be fixed, and also could you please add a mention of this addition in the CHANGELOG.md
file?
The I'll be happy to merge your PR 👍 🙂
Hi @Lucas-C, Should I add documentation for this as well, or would it be better to address it in a new issue? Creating a separate issue might also be a good idea, as it would give me the opportunity to make an additional contribution 🙂 |
We usually integrate the documentation related to new features as part of the PR that introduced it, so yes, it would be a good idea to include some documentation as part of this PR 🙂 Also, have you seen my other previous comments regarding the remaing minor changes required? |
Yes, I have made the changes you suggested. |
I'm sorry but I don't think you adressed those 4 distinct points:
|
I haven't committed it yet. |
Oh OK, alright, that's fine🙂 |
Hi @Lucas-C, I have made the changes and also added documentation. I'm sorry, but I was not able to render the API documentation on my laptop. Please review the code and let me know if any changes are required. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job overall 👍 👍 👍
You wrote really good code, good documentation and good tests!
There are only a few minor details remaining:
- this early comment I made that I think you might have missed: Added Cell level control for Table Borders #1285 (comment)
- an extra line in
CHANGELOG.md
: Added Cell level control for Table Borders #1285 (comment) - some minor improvements in
docs/Tables.md
(I think you could you just batch-apply my code suggestions and this file would be fine)
@Lucas-C please review the commit. |
@allcontributors pleasae add @Kaustbh for code |
I've put up a pull request to add @Kaustbh! 🎉 |
Merged! Thanks a lot for your contribution @Kaustbh 👍 You have been added to the list of |
It was a pleasure contributing and I'm honored to be included in the list of contributors. I am looking forward to collaborating more in the future and contributing to more features and improvements. |
Fixes : #1192
Implemented a new feature that allows granular, cell-level control over table borders. With this update, users can now define specific borders (left, right, top, bottom) for individual cells, offering greater customization when generating tables in PDFs.
Checklist:
The GitHub pipeline is OK (green),
meaning that both
pylint
(static code analyzer) andblack
(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/
folderA 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.