Skip to content

Conversation

@alkaZeltser
Copy link
Collaborator

  • small tweaks to data.table syntax to avoid triggering false positive NOTE re unassigned global vars
  • a statement declaring global variables "ID", ".", "new.ID" whose syntax cannot be changed to avoid the NOTE.
  • I have read the code review guidelines and the code review best practice on GitHub check-list.

  • The name of the branch is meaningful and well formatted following the standards, using [AD_username (or 5 letters of AD if AD is too long)-[brief_description_of_branch].

  • I have set up or verified the branch protection rule following the github standards before opening this pull request.

  • I have added the changes included in this pull request to NEWS under the next release version or unreleased, and updated the date.

  • I have updated the version number in metadata.yaml and DESCRIPTION.

  • Both R CMD build and R CMD check run successfully.

Testing Results

Case 1

All unit tests pass

@alkaZeltser
Copy link
Collaborator Author

@dan-knight so I think I've resolved the data.table syntax NOTE... but the GitHub checker spawned a new NOTE which wasn't in the previous release. It flagged the package size and specifically the /doc directory for exceeding 1 Mb. This is... weird because /doc is not part of the repo. I know it's created when the package is built to store the vignettes, but I haven't edited the vignette since the last release so I don't understand why this is a problem now and not before?

* checking installed package size ... NOTE
  installed size is  5.4Mb
  sub-directories of 1Mb or more:
    doc   5.1Mb

@alkaZeltser
Copy link
Collaborator Author

@dan-knight so I think I've resolved the data.table syntax NOTE... but the GitHub checker spawned a new NOTE which wasn't in the previous release. It flagged the package size and specifically the /doc directory for exceeding 1 Mb. This is... weird because /doc is not part of the repo. I know it's created when the package is built to store the vignettes, but I haven't edited the vignette since the last release so I don't understand why this is a problem now and not before?

* checking installed package size ... NOTE
  installed size is  5.4Mb
  sub-directories of 1Mb or more:
    doc   5.1Mb

Looks like this NOTE can be resolved by compressing vignettes upon building the package:
R CMD build package-ApplyPolygenicScore/ --compact-vignettes="both"

Copy link

@rachelmadang rachelmadang left a comment

Choose a reason for hiding this comment

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

LGTM.

@dan-knight
Copy link
Contributor

Looks like this NOTE can be resolved by compressing vignettes upon building the package: R CMD build package-ApplyPolygenicScore/ --compact-vignettes="both"

Yep. If that's the case, then it's fine to submit as long as you use the compressed build. I should check the CI/CD action to make sure it's using the same setting so we're consistent.

@alkaZeltser alkaZeltser merged commit bd8b1a4 into main Mar 4, 2025
6 checks passed
@alkaZeltser alkaZeltser deleted the nzeltser-fix-datatable-syntax branch March 4, 2025 00:28
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.

4 participants