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

Reorganise code and documentation for mapping tables. #13

Merged
merged 17 commits into from
Aug 26, 2024
Merged

Conversation

inpowell
Copy link
Owner

This pull request splits R/mapping_table.R into separate files:

  • R/BaseMappingTable.R contains definitions for the MappingTable and BaseMappingTable classes.
  • R/MultiMappingTable.R contains the definition for the MultiMappingTable class.
  • R/RangeMappingTable.R contains the definition for the RangeMappingTable class.

Given that the (so-called) "internals" of the classes might be prone to change, the tests for each of these classes have been whittled down.

Documentation for all MappingTable subclasses has been improved, preventing warnings from roxygen2 when documenting classes. Examples have been added for all classes in the documentation.

@inpowell inpowell requested a review from PeterM74 August 24, 2024 04:29
@inpowell
Copy link
Owner Author

(Hint: git diff --color-moved origin/master or similar with recent versions of git really helps understand what has changed, given the file-renames that have occurred.)

@PeterM74
Copy link
Collaborator

I have added a commit.

  • The mapping table documentation was duplicating the title into the description, I have fixed this by specifying the @title and @description settings explicitly rather than letting roxygen (incorrectly) guess.
  • Removed .Rd for the to_dense() function as it's not exported. From my review, there is no need to output documentation for non-exported functions for CRAN checks so I think this should be ok.
  • Some other minor spelling/grammar fixes.

Local check ok
── R CMD check results ────────── modulartabler 0.1.0 ────
Duration: 36.5s

0 errors ✔ | 0 warnings ✔ | 0 notes ✔

@inpowell
Copy link
Owner Author

Thanks for the pick-ups!

@inpowell inpowell merged commit 029d7f7 into master Aug 26, 2024
5 checks passed
@inpowell inpowell deleted the document-maps branch August 26, 2024 00:07
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.

2 participants