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

Convert IRangesOverview.Rnw to IRangesOverview.Rmd #49

Open
wants to merge 8 commits into
base: devel
Choose a base branch
from

Conversation

sonali8434
Copy link

@jwokaty please review this PR I have reviewed it once using the checklist and make sure that R CMD build contains html. After your review we will ask @hpages for final review.

Note: The plots position is quite different in the html document but I believe that in the pdf plots are appeared at the middle of the code blocks which is not possible to generate in html the current position of plots look good to me.

Attached
PDF
HTML for your reference.

@hpages hpages requested review from jwokaty and hpages June 13, 2023 19:17
Make sure that current indents matches the existing document
@sonali8434
Copy link
Author

sonali8434 commented Jun 13, 2023

We will be using this checklist to review my PR I have reviewed it myself, Please review my changes and let me know if any more changes are required any suggestion will be appreciated.

Note: During the review I noticed that pdf has

> if (!require("BiocManager"))
+ install.packages("BiocManager")
> BiocManager::install("IRanges")

I Remember that One of the requirements of a vignette is that it must not install anything without the user's knowledge. But the code label uses eval=FALSE to stop running that code chunk so I think there is no problem.

  • the .Rmd file knits to HTML
  • R CMD build runs without errors or timeouts
  • the tarball from R CMD build contains the HTML (check with the
    following by substituting the package name and vignette name tar ztf package_name.tar.gz | grep 'doc/vignette_name')
  • the .Rnw file has been removed
  • In the DESCRIPTION file
    • BiocStyle and knitr are listed in Suggests
    • the line VignetteBuilder: knitr exists
    • any added lines use the same spacing and indents as the existing document
  • If agreed with the Maintainer
    • the contributor is in the author list in the DESCRIPTION file*.
    • the contributor is in the author list in the vignette's YAML*.
  • If this pull request involves converting from separate Author and Maintainer
    lines to the Authors@R vector, please ensure that
    • the Authors@R vector includes the maintainer as specified with role='cre'.
    • the Maintainer line is completely removed.
  • HTML document is representative of the PDF in content and
    in general the presentation
  • Where the contributor was not able to preserve the content and presentation
    of the PDF is noted as a comment in the pull request
  • the R Markdown file is representative of the Sweave document and follows
    best practices, such as replacing links to Bioconductor packages with calls
    to BiocStyle's Biocpkg()
  • Only files necessary for the conversion are included in the pull request.
  • Long lines have been broken up and reformatted, you can achieve this by
    selecting the text in your script and then clicking on Code > Reflow Comment
    in R Studio.
  • Code blocks are surrounded by a blank line, and there are no blank lines
    before the start or after the end of the code within the code block.
  • Refer to the contribution guide for acceptable formats

Comment on lines 366 to 385

```{r ranges-union}
```

```{r ranges-punion}
```

```{r ranges-intersect}
```

```{r ranges-pintersect}
```

```{r ranges-setdiff}
```

```{r ranges-psetdiff}
```

<!-- \subsection{Mapping Ranges Between Vectors} -->
Copy link
Contributor

@jwokaty jwokaty Jun 15, 2023

Choose a reason for hiding this comment

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

@hpages This is just a suggestion, but maybe this section 366-385 can be removed?

Copy link
Author

Choose a reason for hiding this comment

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

I have not yet removed it. I will remove it after receiving confirmation from @hpages.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, please remove. These are placeholders that were intended to be filled with something at some point but it never happened. Thx!

Copy link
Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

@jwokaty jwokaty left a comment

Choose a reason for hiding this comment

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

Hi @sonali8434 Thank you for your PR. I noticed a few more places where pandoc didn't convert an expression correctly. Otherwise, everything looks really good and it will be ready for Herve after you address these changes.

@sonali8434
Copy link
Author

@jwokaty, thank you for your thorough review. I have incorporated all of your suggested changes, except for this one. Please inform me if any further modifications are needed.

@sonali8434 sonali8434 requested a review from jwokaty June 16, 2023 05:28
Copy link
Contributor

@jwokaty jwokaty left a comment

Choose a reason for hiding this comment

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

Thanks, @sonali8434! I think this ready for @hpages.

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