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.

Sign up for GitHub

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

Jump to bottom

Refactor the report export function to fit in refactored TestEvaluato… #95

Merged
merged 21 commits into from
May 28, 2024
Merged

Refactor the report export function to fit in refactored TestEvaluato… #95

merged 21 commits into from
May 28, 2024

Conversation

tonyshumlh
Copy link
Collaborator

@tonyshumlh tonyshumlh commented May 23, 2024 •
edited
Loading

Refactor the report export function to fit in refactored TestEvaluator codes. Add demo notebook

jinyz8888 and others added 12 commits May 21, 2024 14:39
@jinyz8888
add data presence and data quality checklist
507d251
Merge remote-tracking branch 'origin/add_data_presence_and_data_quali…
be43e31 
…ty_items' into prep-checklist-for-sys-dev
undo the changes in checklist.csv
689ba36
docs: prepare checklist for system development
312c5a0
Merge remote-tracking branch 'origin/84-extend-the-checklists-loader-…
092eac7 
…functionality-to-export-the-checklist-in-htmlpdf-format-for-visualization' into prep-checklist-for-sys-dev
added topic for optional Data Quality
e936aa2
updated example in checklist.py
f18fcc9
fix symbol typo in checklist_sys
dd37cbb
renamed topic
96c705e
added checklist html for visualization
bc6ab8a
Merge branch '84-extend-the-checklists-loader-functionality-to-export…
e2d90af 
…-the-checklist-in-htmlpdf-format-for-visualization' into prep-checklist-for-sys-dev
@tonyshumlh
Refactor the report export function to fit in refactored TestEvaluato…
7a3a13a 
…r codes. Add demo notebook
@tonyshumlh tonyshumlh self-assigned this May 23, 2024
@tonyshumlh tonyshumlh linked an issue May 23, 2024 that may be closed by this pull request
Export report into HTML format #82
Closed
@tonyshumlh tonyshumlh mentioned this pull request May 23, 2024
Closed
SoloSynth1
SoloSynth1 reviewed May 24, 2024
View reviewed changes
Copy link
Collaborator

@SoloSynth1 SoloSynth1 left a comment

Choose a reason for hiding this comment

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

@tonyshumlh Please see comments. Otherwise I think it is quite decent - will merge it once I have resolved the code duplication.

checklist/checklist_sys.html Outdated Show resolved Hide resolved
src/test_creation/modules/workflow/parse.py Outdated Show resolved Hide resolved
SoloSynth1
SoloSynth1 approved these changes May 24, 2024
View reviewed changes
Copy link
Collaborator

@SoloSynth1 SoloSynth1 left a comment

Choose a reason for hiding this comment

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

@tonyshumlh I have done the refactoring we have discussed this afternoon. If you are OK with the changes, let's merge it into main. Thanks!

@tonyshumlh
Copy link
Collaborator Author

tonyshumlh commented May 24, 2024 •
edited
Loading

@JohnShiuMK Tested with the Command Line demo and Notebook demo. Look good to me

jinyz8888
jinyz8888 reviewed May 24, 2024
View reviewed changes
Copy link
Collaborator

@jinyz8888 jinyz8888 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

It throws runtime error when run python src/test_creation/analyze.py checklist_path repo_path . html , python src/test_creation/analyze.py checklist_path repo_path ./report.html pdf andpython src/test_creation/analyze.py checklist_path repo_path ./report.pdf htmlorpython src/test_creation/analyze.py checklist_path repo_path ./report html`

image

@SoloSynth1
Copy link
Collaborator

SoloSynth1 commented May 24, 2024 •
edited
Loading

It throws runtime error when run python src/test_creation/analyze.py checklist_path repo_path . html , python src/test_creation/analyze.py checklist_path repo_path ./report.html pdf andpython src/test_creation/analyze.py checklist_path repo_path ./report.pdf htmlorpython src/test_creation/analyze.py checklist_path repo_path ./report html`

image

Assuming ./report is a directory, this error should be caused by the current program did not check if the existing file is a directory and subsequently handed it over to pandoc. I have pushed a new commit to cover cases where a file is expected but directory is provided, and vice versa.

Now it should return IsADirectoryError instead of RuntimeError, indicating that we are expecting the existing file to be overwritten to be a file.

JohnShiuMK
JohnShiuMK requested changes May 24, 2024
View reviewed changes
Copy link
Collaborator

@JohnShiuMK JohnShiuMK left a comment •
edited
Loading

Choose a reason for hiding this comment

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

pending Orix to fix it
@jinyz8888 to review it

This was referenced May 24, 2024
Closed
Closed
@SoloSynth1
Copy link
Collaborator

checks added to see if the provided filename:

  • contains valid extension; and
  • points to a file/directory, if already exists.

Please feel free to test the commands again - they now should raise our own errors instead of pypandoc's RuntimeError for providing more useful information.

Should be ready to merge now. Will write tests after converting our project into an installable package.

@tonyshumlh
Copy link
Collaborator Author

@JohnShiuMK I tested the functionality. Ready to merge

@tonyshumlh
Copy link
Collaborator Author

@SoloSynth1 there seems a bit formatting issue for Quarto export. I have given some comment

@tonyshumlh
Copy link
Collaborator Author

tonyshumlh commented May 28, 2024 •
edited
Loading

@JohnShiuMK please help with the review and merge. Thank you

JohnShiuMK
JohnShiuMK approved these changes May 28, 2024
View reviewed changes
Copy link
Collaborator

@JohnShiuMK JohnShiuMK left a comment

Choose a reason for hiding this comment

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

all good, thanks!

@JohnShiuMK JohnShiuMK merged commit a1ed0a8 into main May 28, 2024
@JohnShiuMK JohnShiuMK deleted the 82-export-report-refactor branch May 28, 2024 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jinyz8888 jinyz8888 jinyz8888 left review comments

@SoloSynth1 SoloSynth1 SoloSynth1 approved these changes

@JohnShiuMK JohnShiuMK JohnShiuMK approved these changes

Assignees

@tonyshumlh tonyshumlh

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Export report into HTML format
4 participants
@tonyshumlh @SoloSynth1 @JohnShiuMK @jinyz8888