Bioconductor / BiocCheck

http://bioconductor.org/packages/BiocCheck
8 stars 26 forks source link

Roxygen2 generated .Rd files give NOTEs about bad formatting #143

Closed pkrog closed 11 months ago

pkrog commented 3 years ago

I have been annoyed for a long time by formatting NOTEs (indentation and line width) concerning man/*.Rd files generated roxygen2. Do we agree that, since those files are generated, no NOTEs should be displayed? I am willing to make the necessary corrections, so if you are OK, I will propose a pull request.

lshep commented 3 years ago

No, at least I don't agree. While they are recommendations, indentation and line width are good practice and make code more readable. Especially the line width that is suggested so there is not a page run off when displayed online. One suggested alternative is to include linebreaks accordingly.

pkrog commented 3 years ago

No, at least I don't agree. While they are recommendations, indentation and line width are good practice and make code more readable. Especially the line width that is suggested so there is not a page run off when displayed online. One suggested alternative is to include linebreaks accordingly.

With manually written code, yes, but what is the point with generated files? We do not read them or review them.

mtmorgan commented 3 years ago

I think the roxygen man pages follow formatting in the R file? So in the R file you have, e.g., long, unwrapped lines?

vjcitn commented 3 years ago
[2] "Consider shorter lines; 1249 lines (21%) are > 80 characters long."                                                

This is from the submission biodbKegg. I will dig in a little further.

vjcitn commented 3 years ago

It looks like R6 class documentation generation is involved

\if{html}{
\out{<details open ><summary>Inherited methods</summary>}
\itemize{
\item \out{<span class="pkg-link" data-pkg="biodbKegg" data-topic="KeggShape" data-id="draw">}\href{../../biodbKegg/html/KeggShape.html#method-draw}{\code{biodbKegg::KeggShape$draw()}}\out{</span>}
\item \out{<span class="pkg-link" data-pkg="biodbKegg" data-topic="KeggShape" data-id="equals">}\href{../../biodbKegg/html/KeggShape.html#method-equals}{\code{biodbKegg::KeggShape$equals()}}\out{</span>}
\item \out{<span class="pkg-link" data-pkg="biodbKegg" data-topic="KeggShape" data-id="getColor">}\href{../../biodbKegg/html/KeggShape.html#method-getColor}{\code{biodbKegg::KeggShape$getColor()}}\out{</span>}
\item \out{<span class="pkg-link" data-pkg="biodbKegg" data-topic="KeggShape" data-id="getLabel">}\href{../../biodbKegg/html/KeggShape.html#method-getLabel}{\code{biodbKegg::KeggShape$getLabel()}}\out{</span>}
\item \out{<span class="pkg-link" data-pkg="biodbKegg" data-topic="KeggShape" data-id="getRgbColor">}\href{../../biodbKegg/html/KeggShape.html#method-getRgbColor}{\code{biodbKegg::KeggShape$getRgbColor()}}\out{</span>}
}
vjcitn commented 3 years ago

In the R folder

> x = lapply(dir(), readLines)
> sapply(x, function(x) max(nchar(x)))
 [1] 76 80 74 80 80 80 63 80 76 80 74 80 70 80 71 86 76 80 76 76 77  4
vjcitn commented 3 years ago

in man folder

> x = lapply(dir(), readLines)
> sum(sapply(x, function(x)sum(nchar(x)>80)))
[1] 1247

should BiocCheck be doing this?

lshep commented 3 years ago

Seems like a potential bug in the output shown that should be corrected.

But I still would argue we do not want to remove this check; that it should be implemented over the documentation and code.

pkrog commented 3 years ago

I am not suggesting to remove it, but just to do not run the check on the generated files (by checking if the first line contains "% Generated by roxygen2: do not edit by hand"). Moreover this feature could be an option in BiocCheck. Also, if the bad indentation and long lines are not usual in roxygen2 but restricted to R6 Class doc generation, we could write an issue to the maintainers.

LiNk-NY commented 3 years ago

This is more of an issue for roxygen2 where the automatic indentation should be configurable by the user.

LiNk-NY commented 11 months ago

Resolved in 5ee0600