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

Reformat cas #398

Merged
merged 12 commits into from
Jun 2, 2023
Merged

Reformat cas #398

merged 12 commits into from
Jun 2, 2023

Conversation

ethanbass
Copy link
Contributor

@ethanbass ethanbass commented May 8, 2023

  • CAS numbers are automatically reformatted using as.cas in bcpc, cts, etox, flavornet, pan, pubchem, srs functions.
  • Adds verbose argument to as.cas.
  • Changes the behavior of as.cas so it returns a named character vector whose names are the character strings from the original query.
  • Alters test for as.cas (since it now retains the names of the original query).
  • This pull request also changes the way names are assigned to lapply output because I ran into a problem with the way the output was being formatted for "non-cas" cas numbers like "balloon".
  • Updated NEWS

PR task list:

  • [ x] Update NEWS
  • [ x] Add tests (if appropriate) [I didn't add any tests here, but let me know if you want me to add additional tests (e.g. that CAS numbers without dashes are being correctly reformatted).
  • [x ] Update documentation with devtools::document()
  • [ x] Check package passed

ethanbass added 3 commits May 8, 2023 12:33
affects behavior of bcpc, cts, etox, flavornet, pan, pubchem, srs.
- changed the way names are assigned to lapply output because I ran into a problem with the way the output was being formatted for non-cas cas numbers like "balloon".
Copy link
Collaborator

@Aariq Aariq left a comment

Choose a reason for hiding this comment

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

There are a few problems that need fixing, but the concept is great.

R/flavornet.R Outdated Show resolved Hide resolved
R/bcpc.R Outdated Show resolved Hide resolved
R/cts.R Outdated Show resolved Hide resolved
R/cts.R Outdated Show resolved Hide resolved
R/cts.R Outdated Show resolved Hide resolved
R/etox.R Show resolved Hide resolved
R/pubchem.R Outdated Show resolved Hide resolved
@stitam
Copy link
Contributor

stitam commented May 9, 2023

Thanks @ethanbass! Wondering if you folks want this to be part of the next release? I need to resubmit v1.3.0 to CRAN with some comments so I can wait until we merge this PR.

@ethanbass
Copy link
Contributor Author

Thanks @ethanbass! Wondering if you folks want this to be part of the next release? I need to resubmit v1.3.0 to CRAN with some comments so I can wait until we merge this PR.

Whatever you all want to do in this regard is fine with me. I'm not really familiar with your release schedule

R/cts.R Show resolved Hide resolved
R/pubchem.R Show resolved Hide resolved
@ethanbass
Copy link
Contributor Author

Do we want to add additional tests associated with this PR?

@Aariq Aariq requested a review from stitam May 16, 2023 20:53
@stitam
Copy link
Contributor

stitam commented Jun 2, 2023

Not sure why checks on ubuntu-20.04 (devel) passed before and do not pass now, maybe we should update GitHub actions? A possible lead here: https://community.rstudio.com/t/github-action-failure-with-rcmd-check-on-ubuntu-devel/129727. I'm going to merge this PR and fix the check afterwards.

@stitam stitam merged commit e996d7f into ropensci:master Jun 2, 2023
@stitam stitam mentioned this pull request Jun 2, 2023
1 task
@ethanbass ethanbass deleted the reformat_cas branch June 2, 2023 12:56
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