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

208 CRAN requirements for the first submission [skip vbump] #209

Merged
merged 17 commits into from
Sep 8, 2023

Conversation

m7pr
Copy link
Contributor

@m7pr m7pr commented Sep 4, 2023

Closes #208 Closes #201

  • satisfies DESCRIPTION requirements.
  • needed to remove ::: from documentation - this required to
    • update teal.reporter:::ReportCard to teal.reporter::ReportCard as this is exported object
    • update teal.reporter:::Report to teal.reporter::Report as this is exported object
    • delete $append_content() method examples as they used teal.reporter:::NewpageBlock which is unexported
    • delete manual pages for internal, unexported objects: Archiver, ContentBlock, FileArchiver, FileBlock, JSONArchiver, NewpageBlock, PictureBlock, RcodeBlock, Renderer
      • above can still be visible in R/ directory in the source code of classes

@m7pr m7pr added the core label Sep 4, 2023
@m7pr m7pr marked this pull request as ready for review September 4, 2023 11:04
@m7pr
Copy link
Contributor Author

m7pr commented Sep 4, 2023

Tagging @chlebowa @gogonzo and @donyunardi to let you know that internal manual pages will not be accepted on CRAN as they use ::: sign to access non-exported functions.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 4, 2023

badge

Code Coverage Summary

Filename              Stmts    Miss  Cover    Missing
------------------  -------  ------  -------  ----------------------------------------------------------------------------------
R/AddCardModule.R       140       1  99.29%   195
R/Archiver.R             25       0  100.00%
R/ContentBlock.R         18       2  88.89%   38-44
R/DownloadModule.R      207      49  76.33%   89-95, 138, 163-168, 177-181, 184-188, 196-200, 203-207, 214-218, 221-225, 262-266
R/FileBlock.R            13       0  100.00%
R/NewpageBlock.R          2       0  100.00%
R/PictureBlock.R         30       2  93.33%   15, 79
R/Previewer.R           295      56  81.02%   183, 197, 199-202, 205, 208-216, 325-369
R/RcodeBlock.R           15       0  100.00%
R/Renderer.R             84      13  84.52%   95, 103, 112, 114-126
R/ReportCard.R           77       4  94.81%   180, 219, 224, 245
R/Reporter.R             94       1  98.94%   254
R/ResetModule.R          55       0  100.00%
R/SimpleReporter.R       29       0  100.00%
R/TableBlock.R            8       0  100.00%
R/TextBlock.R            13       0  100.00%
R/utils.R                78      66  15.38%   7, 38-97, 99, 102-109
R/yaml_utils.R           74       2  97.30%   41, 239
TOTAL                  1257     196  84.41%

Diff against main

Filename      Stmts    Miss  Cover
----------  -------  ------  --------
TOTAL             0       0  +100.00%

Results for commit: 23a3a45

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

@github-actions
Copy link
Contributor

github-actions bot commented Sep 4, 2023

Unit Tests Summary

    1 files    18 suites   10s ⏱️
195 tests 195 ✔️ 0 💤 0
334 runs  334 ✔️ 0 💤 0

Results for commit fd03911.

♻️ This comment has been updated with latest results.

@m7pr m7pr mentioned this pull request Sep 4, 2023
@m7pr m7pr changed the title #208 CRAN requirements for the first submission 208 CRAN requirements for the first submission Sep 5, 2023
@donyunardi donyunardi changed the title 208 CRAN requirements for the first submission 208 CRAN requirements for the first submission [skip vbump] Sep 5, 2023
@donyunardi
Copy link
Contributor

We will use this PR to track CRAN feedback/update/release and GH release based on our release process steps.

So I will:

  1. Up-version the package (0.2.1) + update PR title to skip vbump.
  2. Built this locally and submit to CRAN
  3. git tag v0.2.1-rc# and push
  4. Address any CRAN feedback and repeat steps 2 and 3 until CRAN approves.
  5. Once in CRAN, then we will merge this to main and tag v0.2.1 for GH release

We probably need to block any PR to main during this time.

@donyunardi
Copy link
Contributor

donyunardi commented Sep 5, 2023

delete manual pages for internal, unexported objects: Archiver, ContentBlock, FileArchiver, FileBlock, JSONArchiver, NewpageBlock, PictureBlock, RcodeBlock, Renderer
above can still be visible in R/ directory in the source code of classes

Since we are removing the .Rd files for internal/unexported objects, should we be more consistent by adding the @noRd tag to all internal and unexported objects and functions?

For example:

#' @keywords internal
report_render_and_compress <- function(reporter, input_list, global_knitr, file = tempdir()) {

If so, perhaps we should make it consistent throughout all teal packages?

@gogonzo
Copy link
Contributor

gogonzo commented Sep 6, 2023

Please remove examples where ::: occurs. I get a response as follows:

Since these examples are for unexported functions and are accessed in their examples, please do not use examples for unexported functions at all. Those functions are only ment to be used internally. If you believe they need examples, then they probably should be exported.


In this PR I see some examples were changed from ::: to :: does it work?

@m7pr
Copy link
Contributor Author

m7pr commented Sep 6, 2023

Hey @gogonzo the change from ::: to :: worked only for exported functions teal.reporter:::ReportCard -> teal.reporter::ReportCard and teal.reporter:::Report -> teal.reporter::Report.

For non-exported functions I will only delete examples and will bring back manual pages (without examples) so noRd tag disappears. We can move examples for non-exported functions to dev/examples.R file, that will be added to .Rbuildignore so it's not added to the sourced package that we will re-submit to CRAN

@m7pr
Copy link
Contributor Author

m7pr commented Sep 6, 2023

Hey, just moved examples do dev/internal_examples.R

@m7pr
Copy link
Contributor Author

m7pr commented Sep 7, 2023

@donyunardi would you like to take care of a resubmission to CRAN?

@donyunardi
Copy link
Contributor

Resubmitted to CRAN as 0.2.1.
Creating tag 0.2.1-rc1.
Waiting for further CRAN feedback.

@m7pr
Copy link
Contributor Author

m7pr commented Sep 8, 2023

Resubmitted again to CRAN as 0.2.1. after we received #208 (comment)

@donyunardi
Copy link
Contributor

donyunardi commented Sep 8, 2023

Create and push v0.2.1-rc2 tag.
Waiting for further feedback.
Hopefully this is it 🤞🏼

@donyunardi
Copy link
Contributor

teal.reporter is now in CRAN:
https://cran.r-project.org/web/packages/teal.reporter/index.html

@donyunardi donyunardi enabled auto-merge (squash) September 8, 2023 17:25
@donyunardi donyunardi merged commit be6a0ef into main Sep 8, 2023
37 of 38 checks passed
@donyunardi donyunardi deleted the 208_cran_comments@main branch September 8, 2023 17:29
m7pr added a commit that referenced this pull request Sep 13, 2023
After package was released on CRAN #209 #208 I would recommend using
CRAN as the main point for our packages installation. r-universe can be
the source for the development version.

This also aims to update readme, as listed in here
insightsengineering/coredev-tasks#462

TODO: gif on the usage of shiny app with teal.reporter module

---------

Signed-off-by: Marcin <133694481+m7pr@users.noreply.github.com>
Co-authored-by: Dony Unardi <donyunardi@gmail.com>
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
m7pr added a commit that referenced this pull request Jan 18, 2024
Close #238 

Examples moved to `dev/` folder during
#209 were
brough back to their places. We substituted `:::` usage, that is
discouraged by CRAN, with `getFromNamespace()` function
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix CRAN issues Release teal.reporter to CRAN
3 participants