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

Fixed coding style issues and adding input checks to functions #41

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

LBreugelmans
Copy link
Collaborator

@LBreugelmans LBreugelmans commented Dec 4, 2024

  • make output of generate_pd_map_and_indicator() silent in readme to avoid showing personal folder path
  • fix improve coding style #33
  • add input checks to start of functions
  • update version
  • run codemeta
  • assign reviewer

To avoid showing personal folder path.
- accidental substitution of MRCA in ape::getMRCA function
- names of list elements not correctly assigned in retrieve_example_data()
@LBreugelmans LBreugelmans requested a review from wlangera December 6, 2024 09:12
@LBreugelmans LBreugelmans marked this pull request as ready for review December 6, 2024 09:12
Copy link
Collaborator

@wlangera wlangera left a comment

Choose a reason for hiding this comment

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

Hi

Thanks for the fast work.
There are some small comments, often because you have more than 80 characters on one line.

It is not really good practice to have all these if statements, this increases the cyclomatic complexity. There is a stopifnot() function and the assertthat package also has some good functions. In the guidelines they have other recommendations.
I will give some examples. Your choice if you want to implement this.

I have updated the package version in the zenodo file

}

# Check that the 'eeacellcode' column exists and does not contain NA
if (any(is.na(mcube$eeacellcode))) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

perhaps use assertthat::noNA()? instead of if statement

proceeding.")
}

if (!"CELLCODE" %in% colnames(grid)) {
Copy link
Collaborator

@wlangera wlangera Dec 9, 2024

Choose a reason for hiding this comment

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

stopifnot (and assertthat):

stopifnot("grid must contain a column named 'CELLCODE' for grid cell codes." = assertthat::has_name(grid, "CELLCODE"))

@wlangera
Copy link
Collaborator

wlangera commented Dec 9, 2024

Please run devtools::document() and codemetar::write_codemeta() after your changes

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.

improve coding style
2 participants