search

JaseZiv / chessR

An R package designed to extract and analyse chess game data played on Lichess and chess.com
https://jaseziv.github.io/chessR/
33 stars 7 forks source link

Repair for CRAN submission #22

Open jonocarroll opened 1 month ago

jonocarroll commented 1 month ago

Please omit the redundant "Functions to" at the start of your title.

You have examples for unexported functions. Please either omit these examples or export these functions. Examples for unexported function get_each_player() in: plot_moves.Rd

\dontrun{} should only be used if the example really cannot be executed (e.g. because of missing additional software, missing API keys, ...) by the user. That's why wrapping examples in \dontrun{} adds the comment ("# Not run:") as a warning for the user. Does not seem necessary. Please replace \dontrun with \donttest. Please unwrap the examples if they are executable in < 5 sec, or replace dontrun{} with \donttest{}.

You write information messages to the console that cannot be easily suppressed. It is more R like to generate objects that can be used to extract the information a user is interested in, and then print() that object. Instead of print()/cat() rather use message()/warning() or if(verbose)cat(..) (or maybe stop()) if you really have to write text to the console. (except for print, summary, interactive functions) -> R/lichess_clock_move_time.R

Please make sure that you do not change the user's options, par or working directory. If you really have to do so within functions, please ensure with an immediate call of on.exit() that the settings are reset when the function is exited. e.g.: ... old <- options() # code line i on.exit(options(old)) # code line i+1 ... options(...) # somewhere after ... e.g.: R/get_raw_lichess.R If you're not familiar with the function, please check ?on.exit. This function makes it possible to restore options before exiting a function even if the function breaks. Therefore it needs to be called immediately after the option change within a function.

Originally posted by @JaseZiv in https://github.com/JaseZiv/chessR/issues/21#issuecomment-2397783932

jonocarroll commented 1 month ago

I'll take a stab at all of these - sorry to see you go the more involved review this time.

JaseZiv commented 1 month ago

I'll take a stab at all of these - sorry to see you go the more involved review this time.

Thanks so much!