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

Fix 12 create report template #109

Merged

Conversation

insightsurge
Copy link
Collaborator

Link to the Issue

Closes #12

Definition of Done

This is an intermediate PR to fix the feature/PR implemented by maria in maria-12-create-report-template

  • Lint fixes and cleaned unnecessary new lines.
  • Directory selection box will appear if report_dir param is NA or an empty string.
    This uses Rstudio API to select a folder as it's OS-independent. utils::choose.dir() only works on windows. This indicates that interactive folder picker will be visible only when user uses rstudio and won't work on other IDEs like VSCode etc. A message for this is also added.
  • Linter defaults added to avoid no visible global function definition lint fail.
  • Message will be shown if user doesn't pass value for report_dir

How to test changes

...

Tasks for PR author

  • Test your change and ensure there is no regression
  • Change has a corresponding issue. Ensure it is linked in GitHub
  • Author of the change opened a pull request and assigned a reviewer

General policy:

  • If applicable - add instructions for testing
  • If there’s no issue, create it. Each issue needs to be well defined and described.
  • All interaction with a user, user-facing messages, plots, reports etc. are written from the perspective of the person using or receiving it. They are understandable and helpful to this person. If a user sees an error message, there is a call to action, i.e. the user knows what to do to fix it.
  • README, other documentation and code comments that we have is updated with all information related to the change.
  • All code has been peer-reviewed before merging into any main branch
  • All changes have been merged into the main branch we use for development.
  • Continuous integration checks (linter, unit tests, integration tests) are configured and pass.
  • Optional: unit tests added for all new or changed logic.
  • All task requirements satisfied. If not describe it here. The reviewer is responsible to verify each aspect of the task.
  • Change covers only things in task. Please create new PR if you want to fix something else.

- rstudioapi dependency added for file picker.
- missing report_dir message added.
- lint fix and code styling fix.
- Added lintr default to avoid "no visible global function definition" warning
Copy link

@ineelhere ineelhere left a comment

Choose a reason for hiding this comment

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

Great work @insightsurge 👍🏼

  • Requirements on DOD are fulfilled.
  • While testing on windows it gives the following error
Structure created at C:\Users\****\AppData\Local\Temp\*****
Switched to develop
cannot open the connectionSwitched back to feature
Warning message:
In file(file, "rt") :
  cannot open file 'C:/Users/****/AppData/Local/Temp/****/tests/cypress/performance.txt': No such file or directory
  

Copy link
Collaborator

@DouglasMesquita DouglasMesquita left a comment

Choose a reason for hiding this comment

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

Thanks for this PR, @insightsurge

I think the improvements in the messages and interaction are good. However, I was not able to generate the report. Maybe I am doing something wrong here. I was not able to do that using the report_dir and also not able to run it manually with create_report function. It seems the structure that the report is expecting is different than the one we are passing as input to the function. Were you able to generate the HTML?

NAMESPACE Show resolved Hide resolved
R/utils_report.R Show resolved Hide resolved
@insightsurge
Copy link
Collaborator Author

It seems the structure that the report is expecting is different than the one we are passing as input to the function. Were you able to generate the HTML?

Yes, I also faced error while generating HTML for report. But we can merge this branch inside maria's branch and cover the rest there as the feature can be really helpful for the end user.

@DouglasMesquita
Copy link
Collaborator

It seems the structure that the report is expecting is different than the one we are passing as input to the function. Were you able to generate the HTML?

Yes, I also faced error while generating HTML for report. But we can merge this branch inside maria's branch and cover the rest there as the feature can be really helpful for the end user.

Agree. I will proceed and merge it then

Copy link
Collaborator

@DouglasMesquita DouglasMesquita left a comment

Choose a reason for hiding this comment

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

We will merge it into fix-12-create-report-template and fix it from there

@DouglasMesquita DouglasMesquita merged commit d4001f3 into maria-12-create-report-template Nov 6, 2023
5 of 10 checks passed
@DouglasMesquita DouglasMesquita deleted the fix-12-create-report-template branch November 6, 2023 08:58
DouglasMesquita added a commit that referenced this pull request Feb 29, 2024
* Added basic template and logic for rendering report

* Moved create report call, added report path creation and copying template

* Updated report with params, yaml error when rendering

* Fixes in the report template

* Fixed bugs

* Fixes in pkg files

* Deleted unused lines

* Fixes for cicd pipeline checks

* Gave user choice for report directory, added messages to report functions, refactored report functions

* Fix 12 create report template (#109)

* file picker added

- rstudioapi dependency added for file picker.
- missing report_dir message added.

* namespace updated

* lint fixes

- lint fix and code styling fix.
- Added lintr default to avoid "no visible global function definition" warning

* dir selection verbose added

* Fix report (#110)

* Removing rstudioapi from the list of dependencies

* adding names to perfomance output list based on the repetition

* Updating benchmark function to work with the new crete_report function

* updating quarto template

* updating and simplifying report auxiliar functions

* Fixing wrong parameter name in create_report call

* running quarto in the destination location (other than root)

* Breaking message into two rows (lintr)

* preventing report to run in case any other problem happens during the benchmark execution

* combining data before sending it to quarto

* adjusting after lintr

* adjusting after lintr

* Making report more robust for different versions

* returning list directly

Co-authored-by: Jakub Nowicki <kuba@appsilon.com>

* using with_dir instead of manually changing working directory

* adding documentation to report utils functions

* updating WORDLIST

---------

Co-authored-by: Jakub Nowicki <kuba@appsilon.com>

* Adding report information to NEWS.md and adding changelog to pkgdown structure

* reorganizing versions in NEWS.md

Co-authored-by: Jakub Nowicki <kuba@appsilon.com>

* adding missing @example tag

---------

Co-authored-by: Maria Drywień <mariadrywien@gmail.com>
Co-authored-by: Douglas R. Mesquita Azevedo <douglasrm.azevedo@gmail.com>
Co-authored-by: Harsh Verma <125121920+insightsurge@users.noreply.github.com>
Co-authored-by: Jakub Nowicki <kuba@appsilon.com>
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