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

simple: fast fixes #64

Merged
merged 3 commits into from
Aug 3, 2021
Merged

simple: fast fixes #64

merged 3 commits into from
Aug 3, 2021

Conversation

colleenXu
Copy link
Contributor

@colleenXu colleenXu commented Jul 19, 2021

should be merged quickly, since these are simple + don't change behavior much + fix a few potential bugs

@coveralls
Copy link

Pull Request Test Coverage Report for Build 1044231511

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 96.831%

Totals Coverage Status
Change from base Build 972320854: 0.0%
Covered Lines: 439
Relevant Lines: 444

💛 - Coveralls

src/config.ts Outdated Show resolved Hide resolved
@newgene
Copy link
Member

newgene commented Aug 1, 2021

@colleenXu For other IDs removed:

remove IDs
BiologicalProcess KEGG
CellularComponent MetaCyc
AnatomicalEntity NCIT
Cell EFO

If the reason to remove MetaCyc, NCIT, EFO is the same as KEGG ("cannot find matching field in the corresponding API"), I would not remove them as well. It's likely the underlying API has an issue to load the corresponding fields, then we should fix the data instead, or find another API.

But if their existence breaks some queries, it's strong enough reason to remove it (or better "comment out") for now.

@colleenXu colleenXu changed the base branch from main to biolink_v2.1 August 2, 2021 03:45
@colleenXu
Copy link
Contributor Author

colleenXu commented Aug 2, 2021

@newgene I have changed the branch, so this will merge to biolink_v2.1 for testing.

With the new commit below, those 3 IDs are back in, with comments that I didn't find those fields in the corresponding APIs. I don't think anything will break by having them in.

CellularComponent MetaCyc, AnatomicalEntity NCIT, Cell EFO
@colleenXu colleenXu merged commit d942b6c into biolink_v2.1 Aug 3, 2021
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.

4 participants