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

Feature/145 chondro replace #156

Merged
merged 61 commits into from
Jun 21, 2020
Merged

Feature/145 chondro replace #156

merged 61 commits into from
Jun 21, 2020

Conversation

eoduniyi
Copy link
Collaborator

@eoduniyi eoduniyi commented Jun 15, 2020

chondro has been replaced with fauxCell in all the .R files (examples; unit test) that reference chondro

Note: vignettes still makes use of chondro

Closes #145

Replace chondro with fauxCell
"lacuna" is not present in fauxCell
Copy link
Collaborator

@bryanhanson bryanhanson left a comment

Choose a reason for hiding this comment

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

@eoduniyi When I try to check this branch I get a message that flu is not exported. Strangely, I don't think you touched flu.R but it does not have an @export statement. Since you can build/check/install perhaps this is a problem with my local files.

@eoduniyi
Copy link
Collaborator Author

@eoduniyi When I try to check this branch I get a message that flu is not exported. Strangely, I don't think you touched flu.R but it does not have an @export statement. Since you can build/check/install perhaps this is a problem with my local files.

Yo u are correct, I did not touch flu.R. Strange, when I make->build->check I get no such messages.

@GegznaV
Copy link
Collaborator

GegznaV commented Jun 16, 2020

Usually, the documentation of dataset should not have @export roxygen tag. In http://r-pkgs.had.co.nz/data.html#documenting-data it is said:

Never @export a data set.

@GegznaV
Copy link
Collaborator

GegznaV commented Jun 16, 2020

@eoduniyi, in r-hyperspec/hySpc.dplyr#23 (comment), I wrote about the "magical" words (fix, fixes, close, closes, resolve, resolves) that make PR to close an issue. These words work only in the main message of PR. So I updated your the main PR (pull request) message with:
image

@GegznaV GegznaV added the Topic: documentation 📘 Related to hyperSpec's (non-vignette) documentation. Use a separate dag for vignettes. label Jun 20, 2020
@GegznaV
Copy link
Collaborator

GegznaV commented Jun 21, 2020

I can confirm, that word chondro is missing in the most of *.R files in this project, except:

  • ✔️ chondro.R: it is expected to be there
  • fauxCell.R: I'm not sure if the new users know what chondro dataset is and, in my opinion, it would be sufficient to mention that this is an artificial dataset that imitates 2D Ramman scattering spectra.
  • ✔️ read.txt.Reinshaw.R: the raw data of chondro dataset is used for unit testing. Will be solved after file IO functions are moved to a separate package.
  • ✔️ spc.fit.poly.R: there is some commented code, that mentions chondro .

image

@GegznaV GegznaV requested review from GegznaV and removed request for GegznaV June 21, 2020 21:27
Copy link
Collaborator

@GegznaV GegznaV left a comment

Choose a reason for hiding this comment

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

As code passes R-CMD-check and chondro was removed from all necessary places, I approve this PR ✔️

@cbeleites cbeleites merged commit ffc3e4a into develop Jun 21, 2020
@GegznaV GegznaV deleted the feature/145-chondro-replace branch June 21, 2020 23:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Topic: documentation 📘 Related to hyperSpec's (non-vignette) documentation. Use a separate dag for vignettes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace chondro in the examples with fauxCell
4 participants