Skip to content

Convert "# nolint" to specific rule exception #178

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 14 commits into from
Feb 14, 2024
Merged

Convert "# nolint" to specific rule exception #178

merged 14 commits into from
Feb 14, 2024

Conversation

averissimo
Copy link
Contributor

@averissimo averissimo commented Feb 8, 2024 •
edited
Loading

Pull Request

Part of #172

Changes description

  • Add new regex style to object_name_linter to account for ADaM variables.
    • See .lintr file
    • regexes = c(ANL = "^ANL_?[0-9]*$", ADaM = "^r?ADSL|ADTTE|ADLB|ADRS|ADAE$"))
      • Allows for variables named ANL with optional underscore and numbers after.
      • note: can't use groups ((rule_within_parenthesis)) due to r-lib/lintr/issues/2188 not being released yet.
  • Convert other # nolint to specific # nolint: <rule name>. for specificity

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

m7pr commented Feb 9, 2024

This is great. I'm all in. For the general .lintr config used in other packages I would assume maybe ADSL_ and other guys can have a suffix with a number, as sometimes we use their copies in examples/vignettes.

@vedhav
Copy link
Contributor

vedhav commented Feb 9, 2024 •
edited
Loading

This is great. I'm all in. For the general .lintr config used in other packages I would assume maybe ADSL_ and other guys can have a suffix with a number, as sometimes we use their copies in examples/vignettes.

Agreed, a common regex of (AD)[A-Z]{2,3} should do it. The datasets are always 4 (usually CDISC defined) or 5 (sponsor defined) characters long.

Base automatically changed from return@178_pre-release-cleanup@main to 178_pre-release-cleanup@main February 9, 2024 11:46
@averissimo
Merge branch 'return@178_pre-release-cleanup@main' into lintr@return@…
d73bf6e 
…178_pre-release-cleanup@main

* return@178_pre-release-cleanup@main:
  use structure to return list with class instead of class(res)<-... and then return res
  removed return( from functions in examples
@averissimo
Merge branch '178_pre-release-cleanup@main' into lintr@return@178_pre…
aafb6ab 
…-release-cleanup@main
@averissimo
Copy link
Contributor Author

averissimo commented Feb 9, 2024 •
edited
Loading

(AD)[A-Z]{1,2}

@vedhav I like it, this would work without the parenthesis `AD[A-Z]{1,2}``

With @m7pr suggestion (although this would allow for rADSL_121 <- 2 wouldn't throw an error):

^r?AD[A-Z]{2,3}(_[0-9]+)?$ or temporary until lintr is bumped ^r?AD[A-Z]{2,3}_?[0-9]*$

@m7pr
Copy link
Contributor

m7pr commented Feb 12, 2024

This is great @averissimo I think you can provide this changes in regexes to make those simpler

@averissimo
Merge branch '178_pre-release-cleanup@main' into lintr@return@178_pre…
29f1c70 
…-release-cleanup@main
@averissimo
Suggestion by @vedhav
f885c57 
Signed-off-by: André Veríssimo <211358+averissimo@users.noreply.github.com>
@averissimo
Copy link
Contributor Author

Done! It's still strict, but flexible enough to account for all ADaM datasets

@averissimo
Merge branch '178_pre-release-cleanup@main' into lintr@return@178_pre…
a35d733 
…-release-cleanup@main
@averissimo
Merge branch '178_pre-release-cleanup@main' into lintr@return@178_pre…
5af40da 
…-release-cleanup@main

Signed-off-by: André Veríssimo <211358+averissimo@users.noreply.github.com>
m7pr
m7pr approved these changes Feb 13, 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.

Love the push! Would you mind creating a thread for the wider discussion on application of this in other packages?

@averissimo
Merge branch '178_pre-release-cleanup@main' into lintr@return@178_pre…
bb7423d 
…-release-cleanup@main

* 178_pre-release-cleanup@main:
  remove unused test functions
@averissimo
Copy link
Contributor Author

Done: https://github.com/insightsengineering/coredev-tasks/issues/509

@m7pr
Copy link
Contributor

m7pr commented Feb 14, 2024

I think this is ready to be merged

@averissimo
Merge branch '178_pre-release-cleanup@main' into lintr@return@178_pre…
9a879fe 
…-release-cleanup@main

Signed-off-by: André Veríssimo <211358+averissimo@users.noreply.github.com>
@averissimo averissimo merged commit f2d64a4 into 178_pre-release-cleanup@main Feb 14, 2024
@averissimo averissimo deleted the lintr@return@178_pre-release-cleanup@main branch February 14, 2024 09:26
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

@vedhav vedhav vedhav left review comments

@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 @vedhav