inbo / checklist

An R package for checking R packages and R code
https://inbo.github.io/checklist
GNU General Public License v3.0
19 stars 2 forks source link

option to allow other word delimiters in folder (file) names? #43

Closed hansvancalster closed 3 years ago

hansvancalster commented 3 years ago

I believe it can be quite common to also have - in folder names, possibly in combination with _. Currently, only _ is allowed.

Examples:

I believe an extra argument to check_filename() that states which word delimiters are allowed (default only _ underscore; and hyphen or both as the only alternatives) would greatly increase the chance that existing repositories consider using the checklist package.

What do others think about this? @inbo/bmk ?

_Originally posted by @hansvancalster in https://github.com/inbo/checklist/pull/38#discussion_r616791097_

ThierryO commented 3 years ago
hansvancalster commented 3 years ago
  • Keep in mind that you should only test folder names in the source code, not the rendered output. Hence you are free to use what ever folder name you want in the output.

I know, but several of the examples I gave refer to source code.

  • Allows different rules across repositories makes things more complex when working on different repositories. You'll have to remember with rules apply to a specific repository. Having the same rules for every repository makes it easier.

I agree with this completely for R packages (I forgot to mention in the issue that I would not allow this option for R packages, only for source repos). For repositories that only store source code and which are often only used in a project-specific setting, I think more flexibility would be a bonus.

ElsLommelen commented 3 years ago

What do others think about this? @inbo/bmk ?

I'm in favour of using 1 separator. Certainly for - and _, I find quite confusing if they are mixed: they look very similar, and I find myself making a lot of mistakes if I for instance have to refer to filenames with both separators mixed together.

hansvancalster commented 3 years ago

I just stumbled on this after running checklist::check_source()

image

So, if we also put the .Rproj file under version control (which is often done to make sure everyone has the same Rproj settings), then checklist expects the projectname to adhere to the filename conventions. By default, Rstudio will use the GitHub repository name as the projectname. The convention at INBO for repository names is to use a hyphen as separator instead of an underscore (if I remember correctly: @peterdesmet ?).

peterdesmet commented 3 years ago

The convention at INBO for repository names is to use a hyphen as separator instead of an underscore (if I remember correctly: @peterdesmet ?)

That is correct. I support your proposal to have - as allowed separator for directory/file names.

hansvancalster commented 3 years ago

The convention at INBO for repository names is to use a hyphen as separator instead of an underscore (if I remember correctly: @peterdesmet ?).

@ThierryO it's actually in a tutorial we wrote: styleguide repos. The tutorial advices to use dash instead of underscore (for both directory and file names) and thus contradicts the checklist naming convention.

ElsLommelen commented 3 years ago

@hansvancalster So function names have to use underscores, and we would not be allowed to use the function name (with underscores) as filename as well, because we have to use dashes in filenames? Sounds a bit confusing, because this styleguide advises to use underscores for function names (or object names in general) and to use the same name for their file (see under functions a bit lower).

I already changed my style from camelcase to snake case some years ago (after this first styleguide was written), I'm not in favor of changing the style again (or have some more aberrant packages) because a newly added styleguide (the one you mention) suddenly adds again another style than the one that has now been used for several years (the one I mention, it is developed while we were still at Kliniekstraat). For me these regular changes in style mean every time 3 or 5 more package that are not conform with novel INBO rules (or a lot of work to adapt them), and no access to easy testing tools such as checklist. So I would ask: please make sure to stay at least conform to previous styleguides when you add a new one! This would at least avoid this type of discussions. (Unless maybe when you have a serious reason why INBO should make the change, of course other than a reason 'person xxxx advises this', because there are a lot of R programmers and everyone has his/her preferences...)

hansvancalster commented 3 years ago

I understand your frustration: it is the very reason why I started this discussion 😉 But please note that I am nowhere suggesting that you have to use dashes in filenames. I am merely pointing out inconsistencies and only asking for more flexibility in checklist::check_source() (not checklist::check_package()! see https://github.com/inbo/checklist/issues/43#issuecomment-826613539). That would only mean the function would gain an extra argument to allow dash as separator in addition to underscore, but the default would be underscore. I am all in favor of clear and consistent guidelines, but I still think there are enough arguments to consider this. To be very clear: I really ❤️ the checklist package and will be a most vivid advocate of its use (with default values whenever reasonable).

ThierryO commented 3 years ago

The convention at INBO for repository names is to use a hyphen as separator instead of an underscore (if I remember correctly: @peterdesmet ?).

@ThierryO it's actually in a tutorial we wrote: styleguide repos. The tutorial advices to use dash instead of underscore (for both directory and file names) and thus contradicts the checklist naming convention.

No. The blame indicates that OSciBio wrote that document and added you and me as author. I never contributed to that document.