Skip to content

Removes return call if it is the last expression #177

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
Merged
merged 4 commits into from
Feb 9, 2024
Merged

Removes return call if it is the last expression #177

merged 4 commits into from
Feb 9, 2024

Conversation

@averissimo
Copy link
Contributor

@averissimo averissimo commented Feb 8, 2024 •
edited
Loading

Pull Request

Part of #172

Simple change, but I created PR since it touches code

Changes description

  • Removes return(...) call if it is the last expression
  • Per NEST guidelines

@averissimo averissimo added the core label Feb 8, 2024
@averissimo averissimo mentioned this pull request Feb 8, 2024
Merged
27 tasks
m7pr
m7pr approved these changes Feb 9, 2024
View reviewed changes
Copy link
Contributor

@m7pr m7pr left a comment

Choose a reason for hiding this comment

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

Hey @averissimo I pushed a commit where I also removed return( from functions used in examples. I hope this is fine. It was quicker than pointing this out and waiting for your reaction.

This is ready to be merged. One last comment, which you can omit, should this part

https://github.com/insightsengineering/teal.transform/pull/177/files#diff-adc2a0beb132b0d6c4a979ad543537e20d967151b972faf0e051fb9e5152df22R142-R146

be changed to return(structure( instead of assigning out and then returning return(out?

@averissimo
Copy link
Contributor Author

averissimo commented Feb 9, 2024

Good call @m7pr !! thanks.

I took another pass and found a few others that could be simplified 8ce0dc6

res <- list(...)
class(res) <- "something"
res

to

structure(
  list(...),
 class = "something"
)

@m7pr
Copy link
Contributor

m7pr commented Feb 9, 2024

Looks great, I think you can merge

@averissimo averissimo merged commit 34baa11 into 178_pre-release-cleanup@main Feb 9, 2024
@averissimo averissimo deleted the return@178_pre-release-cleanup@main branch February 9, 2024 11:46
averissimo added a commit that referenced this pull request Feb 14, 2024
@averissimo @dependabot-preview
@kartikeyakirar @m7pr @github-actions
Pre-release activities (prepare for CRAN release) (#176)
f3c05c6 
# Pull Request

<!--- Replace `#nnn` with your issue link for reference. -->

Fixes #172 

### Summary

* Review and update:
    * [x] README.md (check example code)
    * [x] NEWS.md
      * one liner change
* Review functions:
    * [x] @example tag, make sure it runs, fix if otherwise
* [x] Make sure functions has @return tag to document the return value
    * [x] no \dontrun tag, replace with if(interactive()) if needed
      * `merge_datasets` example is insufficient
* [x] Package `Title` is not duplicated in Package `Description` in
DESCRIPTION file (e.g. this happens in teal.slice currently)
* [x] You have checked the Package Release Template
https://github.com/insightsengineering/teal.reporter/pull/205/files
* [x] Make sure there are no `:::` in examples
* if you need to retain the example that uses `:::`, use
`getFromNamespace()` function.
* [x] remove package:: call and depend package:: call from example.
@kartikeyakirar
* [x] Make sure all `teal.*` mentions are lower-cased and quoted
* [x] Make sure each link to our documentation hosted with pkgdown on
github pages do not have `/main/` in the address
  * it should have has `/latest-tag/` instead
* so we always expose the documentation of the latest release and not
what's currently on main branch but not yet released
* [x] Remove old rd syntax
* [x] Switch from title case into sentence case for title and
description of functions.
* [x] All package names in `Title` and `Description` fields of
DESCRIPTION file are quoted with `'` _(not backtick)_
* [x] Sanity check of all vignettes, make sure there is no typo, no
wrong format, etc. @kartikeyakirar

#### New

* [x] Remove prefixes from data calls `rADRS`, `rADTTE`, etc... (just
like `{teal.data}`)
* [x] Remove `return` wrapper if it is the last expression (per NEST
guidelines)
  * #177
* [x] Remove  exception in `.lintr`: `indentation_linter = NULL` 
* [x] ~Test for unused functions (in package and overall in NEST)~
@averissimo
    * PR request against this feature branch 
* 🛑 This should not be done on this release as it may have unintended
consequences.
* [See this
comment](#176 (comment))
with possible candidates
 * [x] Standard order of roxygen2 tags 
   * `@title ➡️ @doctype ➡️ @description ➡️ @details ➡️ @Rdname ➡️ `
* `➡️ @inheritParams ➡️ @params ➡️ @return ➡️ @Seealso ➡️ @references
➡️`
   * `➡️ @examples ➡️ @export ➡️ @Keywords  ➡️  @noRd`
* [x] Remove `@noMd` (in favor of `Roxygen: list(markdown = TRUE)` in
`DESCRIPTION`)

#### To consider?
* [x] Convert "# nolint" to specific rule exception
  * #178
* [x] ~Added `resolve()` to the list of exported functions as the
sub-functions are exported (`resolve.delayed_variable_choices`, ...).~
  * Let's discuss if this needs to be reverted.
* Edit: this is a trick to have S3methods internally as `resolve` should
not be exported see aff0f35

#### Move to its own issue

* [x] Make sure non-exported functions do not have examples
  * #181

#### After checklist is completed _(🚨 blocked until checklist is
completed)_
* [x] Run urlchecker::url_check() to identify broken links and fix
* [x] Run R CMD check --as-cran make sure everything pass
* [x] Make Sure `inst/WORDLIST` is minimalized

#### Content review _(🚨 blocked for now)_

All tasks above are mostly focused on structure, standards and cleanup.
Here is to look at content

What to look for:

* Review content of titles, descriptions, params, etc.
  * They should be clear to the reader 
* Review vignettes
  * Review content
  * All chunks of code are runnable

Content-related tasks should have a PR against this feature branch.

---------

Signed-off-by: André Veríssimo <211358+averissimo@users.noreply.github.com>
Signed-off-by: Marcin <133694481+m7pr@users.noreply.github.com>
Signed-off-by: kartikeya kirar <kirar.kartikeya1@gmail.com>
Co-authored-by: 27856297+dependabot-preview[bot]@users.noreply.github.com <27856297+dependabot-preview[bot]@users.noreply.github.com>
Co-authored-by: unknown <kirar.kartikeya1@gmail.com>
Co-authored-by: m7pr <marcin.kosinski.mk1@roche.com>
Co-authored-by: Marcin <133694481+m7pr@users.noreply.github.com>
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@m7pr m7pr m7pr approved these changes

Assignees

No one assigned

Labels

core

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@averissimo @m7pr