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

1992 corfunc perspective needs export and report capabilities #2065

Merged

Conversation

lucas-wilkins
Copy link
Contributor

This is an early pull request for part of my implementation of reports for corfunc (issue #1992).

As it stood, each perspective was treated differently, the main content of this pull request is a base class for all the perspectives that contains functionality that they should all support. This includes the getReport method.

As you will see, I have left many TODO notes in the new class, and in some of the perspectives too. The plan is to implement these at the end dealing with the current issue. These are mostly renaming, consistent use of getters/setters, removing some unused bits of code etc, it should be pretty easy now that they reference a common base class.

There is one TODO note that would be part of the task of disentangling GUI stuff from the rest of the code.

Additionally, I have converted a list of objects into a named (and typed) tuple called ReportData, and added some typing surrounding it.

@lucas-wilkins lucas-wilkins added Reports Concerns report functionality Housekeeping Tidying, renaming, formatting, minor refactoring, boring stuff in general labels Jun 9, 2022
@lucas-wilkins lucas-wilkins linked an issue Jun 9, 2022 that may be closed by this pull request
I have also moved some stuff that was split into 3 functions into 1, as it made following the code pretty hard
@lucas-wilkins
Copy link
Contributor Author

Final bits are now done - @smk78, @wpotrzebowski and whoever else is interested.

Most of my additions are now in a folder called reports.

I have now included a couple more packages, which I've added to requirements.txt, but nowhere else:

html2text -> for converting html to text, instead of using regex (html is not a regular language, regular expressions are not appropriate). This package is old, but it was written by Aaron Swartz - RIP, and the text produced is actually RST, so with the appropriate renderer the text output will show proper tables and even figures.

importlib-resources and importlib_resources -> standard package for correctly addressing non-python files within a python package. If we are sure we will be in python >3.7, then only the former is needed.

Copy link
Contributor

@rprospero rprospero left a comment

Choose a reason for hiding this comment

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

Just a couple of minor optional stylist suggestions

@wpotrzebowski wpotrzebowski added the Discuss At The Call Issues to be discussed at the fortnightly call label Jul 4, 2022
@wpotrzebowski wpotrzebowski removed the request for review from m2cci-NMZ July 5, 2022 13:40
@wpotrzebowski wpotrzebowski removed the Discuss At The Call Issues to be discussed at the fortnightly call label Jul 5, 2022
Copy link
Contributor

@krzywon krzywon left a comment

Choose a reason for hiding this comment

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

This cleans up perspectives quite a bit and does what it says it does by adding report capabilities to the corfunc perspective. I'd suggest looking at the one error I saw, otherwise, I have a number of suggestions, but those could always be TODOs.

Copy link
Contributor

@krzywon krzywon left a comment

Choose a reason for hiding this comment

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

This is ready to be merged. The two outstanding suggestions I have should be documented in their own feature requests, assuming they aren't implemented here.

@@ -74,7 +74,8 @@ jobs:
python -m pip install numpy scipy==1.7.3 docutils "pytest<6" sphinx unittest-xml-reporting
python -m pip install tinycc h5py sphinx pyparsing html5lib reportlab==3.6.6 pybind11 appdirs
python -m pip install six numba mako ipython qtconsole xhtml2pdf unittest-xml-reporting pylint
python -m pip install qt5reactor periodictable uncertainties debugpy
python -m pip install qt5reactor periodictable uncertainties debugpy dominate
Copy link
Member

Choose a reason for hiding this comment

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

At least on windows, this also needs importlib_resources and html2text

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, yeah, that needs to be done. Will fix when I'm back at work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in latest push, if you're happy with it, merge it, I think its time.

Copy link
Member

Choose a reason for hiding this comment

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

thanks, go ahead

@rozyczko
Copy link
Member

rozyczko commented Jul 8, 2022

Currently, the two reports: from Fitting and from Corfunc look rather different, having non-matching font, layout, section names etc.
Should we strive to be at least a bit more consistent?

repors

@lucas-wilkins
Copy link
Contributor Author

lucas-wilkins commented Jul 8, 2022 via email

@lucas-wilkins
Copy link
Contributor Author

#2075

Currently, the two reports: from Fitting and from Corfunc look rather different, having non-matching font, layout, section names etc. Should we strive to be at least a bit more consistent?

#2075

@lucas-wilkins lucas-wilkins merged commit 465b774 into main Jul 18, 2022
@krzywon krzywon deleted the 1992-corfunc-perspective-needs-export-and-report-capabilities branch August 18, 2022 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Housekeeping Tidying, renaming, formatting, minor refactoring, boring stuff in general Reports Concerns report functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Corfunc perspective needs export and report capabilities
6 participants