-
Notifications
You must be signed in to change notification settings - Fork 2
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
Coding standards/misc #11
Comments
Thanks @gvegayon for all the valuable comments! 👏 ad 1) Thanks for pointing this out. Did not know about that recommendation yet. I would change this in all my projects later. Is this a blocker in your eyes? |
Not at all, as I said, it is about style. I do think that HW's suggestion should be a standard overall, but if your package works and CRAN is OK with it, I should be OK as well.
Why is that? I'm not a ggplot2 user, but I think there is no requirement for plot.[someclass]... it is actually very flexible. In the case of ggplot2 it suffices with returning the ggplot object and R will do the rest (no need of explicitly calling print). Here is an example of an R package that I've been working on that uses ggplot2 (actually ggtree) in a plot S3 method. Furthermore. I've just submitted a pull request with this so you can take a look at it: #12
Cool!
You are welcomed Also, I just realized that there is no manual for mcMST or mcMST-package. I suggest you create one... That would be #5 Also (2), I added a ChangeLog... I think these are useful HIH |
Thanks a lot @gvegayon! Merged.
Just added docs for the package via 9dfbeaf 👍
Thanks! I guess everything is fine now 😃 |
Indeed |
I know that there is no golden rule/book of coding conventions in R. But I just thought about mentioning these that you can consider to add in your project (which overall looks very good 👍 ), plus a couple of other misc comments:
Following Hadley Wickham's R-pkgs
http://r-pkgs.had.co.nz/namespace.html
I would use the
::
notation instead of, for example,requirePackages
in plotGraph.R I would directly callggplot2::ggplot
and friends.Since you already have the print method for
mcGP
objects, you can introduce theplot.mcGP
method as well instead of the plotGraph function. Sort of the same thing applies with the objects of classecr_multi_objective_result
ecr_result
I miss some examples in:
edgeListToCharVec
,permutationToCharVec
,permutationToEdgelist
,prueferToCharVec
Not an expert in the problem of multiple objective minimum spanning tree problems, but after googling a on CRAN I found these packages that are related and perhaps can be nice to include around references:
The text was updated successfully, but these errors were encountered: