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

Remove KEGGREST dependency #113

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Remove KEGGREST dependency #113

wants to merge 12 commits into from

Conversation

Aariq
Copy link
Collaborator

@Aariq Aariq commented Aug 6, 2024

Closes #108 by accessing KEGG API directly with httr2 (already a dependency) for getting mol files.

@Aariq
Copy link
Collaborator Author

Aariq commented Aug 6, 2024

devtools::check() passes locally on my mac, so I think the failing test is OK to ignore

@KristinaRiemer
Copy link
Collaborator

I'm not sure if this is a standard thing to do or necessary, but is there a way to include a reference to the KEGGREST function being the original inspiration besides in NEWS.md?

@Aariq
Copy link
Collaborator Author

Aariq commented Aug 7, 2024

I'm not sure if this is a standard thing to do or necessary, but is there a way to include a reference to the KEGGREST function being the original inspiration besides in NEWS.md?

I'll include an @note in the documentation for get_mol_kegg() pointing users to KEGGREST if they need more

@Aariq Aariq requested a review from KristinaRiemer August 13, 2024 18:09
@Aariq Aariq mentioned this pull request Dec 9, 2024
10 tasks
Copy link
Collaborator

@KristinaRiemer KristinaRiemer left a comment

Choose a reason for hiding this comment

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

I ran get_mol_kegg(c("C16181", "C00042"), dir = "temp_mols/") from the main and this PR to compare the resulting mol files and they were the same.

This returns two mol files still, each with a name that is the compound id, but they both have the mol files for both compounds in each. Is that the desired behavior @Aariq?

@Aariq
Copy link
Collaborator Author

Aariq commented Dec 9, 2024

I ran get_mol_kegg(c("C16181", "C00042"), dir = "temp_mols/") from the main and this PR to compare the resulting mol files and they were the same.

This returns two mol files still, each with a name that is the compound id, but they both have the mol files for both compounds in each. Is that the desired behavior @Aariq?

Nope! They should be split. I'll take a look. Thanks for the example.

@Aariq
Copy link
Collaborator Author

Aariq commented Dec 9, 2024

I ran get_mol_kegg(c("C16181", "C00042"), dir = "temp_mols/") from the main and this PR to compare the resulting mol files and they were the same.
This returns two mol files still, each with a name that is the compound id, but they both have the mol files for both compounds in each. Is that the desired behavior @Aariq?

Nope! They should be split. I'll take a look. Thanks for the example.

I had a glue::glue_collapse() in there for reasons I can't remember that was combining all the compounds into one. Added a test for this and it should be working now

@Aariq Aariq requested a review from KristinaRiemer December 9, 2024 22:46
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.

Drop KEGGREST as a dependency?
2 participants