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

@agisga 2nd round #26

Closed
lrnv opened this issue Nov 19, 2020 · 0 comments · Fixed by #27
Closed

@agisga 2nd round #26

lrnv opened this issue Nov 19, 2020 · 0 comments · Fixed by #27

Comments

@lrnv
Copy link
Owner

lrnv commented Nov 19, 2020

@lrnv Generally the code changes look good. Here are a few comments based on my previous feedback and the new changes:

  • Vignette 4 still has syntactically wrong and redundant two lines of code here:
  • In issue API fixes #11 you say that "the CamelCase and snake_case switches are to seperate constructors and standard (r/d/p/v)-functions from methods and the rest. Take a look at the new reference, you will see that the common uses of the two standards makes sense now that it's ordered." It would be good to mention these or other coding/syntax/style standard (if any) that you follow under the contribution guidelines in the README (or a separate file if it's too much information for the README).
  • The new version of the cort package works flawlessly on my system. However, the first time I tried it, I was getting error messages, which went away after I upgraded furrr to the newest version (from CRAN). I'm not sure what version of furrr I had prior to upgrading. Maybe it would be good to provide version requirements for furrr and all other dependencies in the DESCRIPTION (currently, testthat has a version requirement stated, but other dependencies do not)?

I also had a very quick look at the new changes to the paper:

Originally posted by @agisga in openjournals/joss-reviews#2653 (comment)

@lrnv lrnv linked a pull request Nov 19, 2020 that will close this issue
@lrnv lrnv closed this as completed in #27 Nov 19, 2020
lrnv added a commit that referenced this issue Nov 19, 2020
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 a pull request may close this issue.

1 participant