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

New pdf report #4030

Merged
merged 9 commits into from
Jan 8, 2024
Merged

New pdf report #4030

merged 9 commits into from
Jan 8, 2024

Conversation

maxcapodi78
Copy link
Collaborator

Entirely based on Python

@ansys-reviewer-bot
Copy link
Contributor

Thanks for opening a Pull Request. If you want to perform a review write a comment saying:

@ansys-reviewer-bot review

Copy link

codecov bot commented Jan 5, 2024

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (03f7b36) 81.42% compared to head (07fc691) 81.46%.
Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4030      +/-   ##
==========================================
+ Coverage   81.42%   81.46%   +0.04%     
==========================================
  Files         182      182              
  Lines       63380    63565     +185     
==========================================
+ Hits        51607    51784     +177     
- Misses      11773    11781       +8     

Copy link
Collaborator

@SMoraisAnsys SMoraisAnsys left a comment

Choose a reason for hiding this comment

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

Good job with this refactoring ! I do have a warning to raise and a couple of minor changes.

Warning: you use f-strings (which is nice) but I don't know if this feature should be used by our IronPython users. If that's the case, you should this and use str format to be compatible. If that's not the case, I would recommend using dataclasses for the classes at the beginning of the file pdf.py. That would make total sense here and you could just use MyDataClass(**kwargs).

Minor changes:

  • use private method and attribute instead of protected (__ vs _) except for self._outline which comes from FPDF;
  • move import at the begining of a function

pyaedt/generic/pdf.py Show resolved Hide resolved
pyaedt/generic/pdf.py Outdated Show resolved Hide resolved
pyaedt/generic/pdf.py Outdated Show resolved Hide resolved
pyaedt/generic/pdf.py Outdated Show resolved Hide resolved
pyaedt/generic/pdf.py Outdated Show resolved Hide resolved
pyaedt/generic/pdf.py Outdated Show resolved Hide resolved
pyaedt/generic/pdf.py Outdated Show resolved Hide resolved
pyaedt/generic/pdf.py Outdated Show resolved Hide resolved
pyaedt/generic/pdf.py Outdated Show resolved Hide resolved
pyaedt/generic/pdf.py Outdated Show resolved Hide resolved
maxcapodi78 and others added 5 commits January 8, 2024 09:53
Merged templatedata and ReportSpecs into a single data class
Merged templatedata and ReportSpecs into a single data class
Copy link
Member

@Samuelopez-ansys Samuelopez-ansys left a comment

Choose a reason for hiding this comment

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

Really great! It is worth adding it somewhere in the documentation for better feature visibility.

@maxcapodi78 maxcapodi78 merged commit cb96fed into main Jan 8, 2024
15 checks passed
@maxcapodi78 maxcapodi78 deleted the new_pdf_report branch January 8, 2024 12:53
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.

3 participants