Skip to content

Conversation

@stineb
Copy link

@stineb stineb commented May 18, 2020

Added some links to section Simulation and Modelling.

Copy link
Member

@mmaelicke mmaelicke left a comment

Choose a reason for hiding this comment

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

Hey,
Thanks for submitting! Some really interesting stuff!

I would suggest splitting this a bit into several PRs, because some of the comments the reviewers might have do not apply to all packages.

Specificly to rpmodel I have some minor comments:

  • The installation example given on README.md does not work. I think because some of the commas got lost on line break.
  • In the docs I found a HTML file that seems to be a GPL license, right? Could you place one into the repo as text file as well? This will make Github recognize the License and the info more readily available.
  • a bit trivial, but nevertheless: Is it possible to place the default installation command into the README as well? For just occasionally-R users like myself this can be really helpful.

I didn't have a close look on the others.
Thanks again for sharing!

@schymans
Copy link
Member

Yes, I think it is even in the check list that we only suggest one addition per PR. This is how the awesome geoscientists do it and it makes sense, so that we don't block a whole PR because of one unclear item.

@stineb
Copy link
Author

stineb commented May 29, 2020

@mmaelicke, thanks for reviewing. I'll create separate PRs for each added line.

Regarding the license for the rpmodel package, I'm following CRAN standard. The license is defined in DESCRIPTION and is a standard license (GPL-3). When building R package websites, the license file you see in /docs is automatically created. I won't change this.

Regarding installation example for the rpmodel package: I checked it again and it works for me. What error do you get?

@stineb
Copy link
Author

stineb commented May 29, 2020

I added separate lines with separate commits and started another pull request. But it seems to take all three commits at once when I create the PR. Please consider taking them at once. Thanks.

@mmaelicke
Copy link
Member

mmaelicke commented May 29, 2020

Hey there,

rpmodel

Regarding the license:

Yeah and I finally found the license information. My suggestion was just to add a file called LICENSE into the root of the repository and not changing the packaging, as Github will recognize the license then. Your suggested link is referencing the Github repository and not CRAN package. Consider this example
image
where the license information is available on the landing page, without the need to navigate somewhere, as it is pretty common on Github. But this is only a suggestion and I am fine with not changing. If you want to have that accessibility, you can also conveniently create a file called LICENSE on Github and it will suggest templates automatically, including the GPL-3. It does not affect the CRAN packaging.

Error message

Well, I run into this error if I just copy&paste the example:

image

I think the problem are the commas, which are commented out. If you put them in front of the comments, the copy&paste works as intended:

image

I cropped the output a bit, but with the commas in place it working fine.

tests

What I could not find at all to put the awesome badge following awesome.md onto this link are tests or a hint how the code can be tested, if unittests don't apply, please guide me, if I missed that part.

plantecophys

This module looks absolutely awesome, found everything needed to place the badge.

bigleaf

install.packages("bigleaf") 

exits with non-zero exit code in a R 3.6 environment and I couldn't find any version limitations. I already dropped an issue there to report.

Thank you,
Best
Mirko

@schymans
Copy link
Member

schymans commented Aug 7, 2020

@stineb, thanks a lot for contributing this. To move forward, how about creating new branches starting from master and adding one of the items in each and then create a separate merge request for each? If something is missing for the awesome badge in one of the projects, we could always included without the badge for the time being and add it later.

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.

3 participants