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

Semi-finally rename interface elements #235

Closed
jsstevenson opened this issue Aug 17, 2023 · 7 comments
Closed

Semi-finally rename interface elements #235

jsstevenson opened this issue Aug 17, 2023 · 7 comments
Labels
enhancement New feature or request priority:low Low priority

Comments

@jsstevenson
Copy link
Member

jsstevenson commented Aug 17, 2023

  • create_db -> get_db or get_db_connection
  • give the q parameter a different/longer name
  • change the "incl"/"excl" params, more verbosity is probably better. Maybe sources= and exclude_sources=
  • Maybe update the whole QueryHandler class name
  • define fastapi routes in routes.py instead of main.py
  • define an entry point to start uvicorn at gene_norm_serve or something to that effect (or maybe provide different CLI options for gunicorn/uvicorn/etc)
  • etl properties like "src_dir", "src_data_dir" etc. I have to check what we meant by the names basically every time... we could probably come up with some more descriptive variable names now.
    • IMO we can probably get rid of some of the ETL initialization args entirely. Like, we have to hard code a bunch of directory paths for different files, in what world is there going to be an alternate hostname to ftp.ncbi.nlm.nih.gov that has the exact same directory structure
  • Make a final determination about whether metadata should link directly to the file used or to a container for the file
  • "search" is potentially kind of ambiguous
  • I am a little concerned about "gene" as a python package name and worry about potential import namespace issues with other stuff in this space (would be solved by a good package rename)
  • Not really a renaming thing, but constructing the QueryHandler class w/ no params should just call the create_db function by default
  • Make sure custom exceptions are named "Error" not "Exception"
  • In search, the way to include and exclude source names is kind of clumsy
  • gene.database.database gives from datetime import datetime vibes. not desirable.
  • do the above for all normalizers
@jsstevenson jsstevenson added enhancement New feature or request priority:low Low priority technical debt A feature/requirement implemented in a sub-optimal way & must be re-written. Contrast to "cleanup" and removed technical debt A feature/requirement implemented in a sub-optimal way & must be re-written. Contrast to "cleanup" labels Aug 17, 2023
@jsstevenson
Copy link
Member Author

related: #237

@korikuzma
Copy link
Member

korikuzma commented Aug 21, 2023

  • Rename Location classes to something other than ChromosomeLocation and SequenceLocation or just create functions. UPDATE: I think these should just be functions

@korikuzma
Copy link
Member

I can't remember the last time I actually looked at gene-normalizer, but looking at it now makes me realize it could use a nice refactor. Seems like a lot of the code was initially done 3 years ago and there are better ways to write / maybe even code that isn't necessary

@jsstevenson
Copy link
Member Author

The various concept names like "BaseGene", "Gene", and then the VRS 2.x "Gene" should be renamed to disambiguate them more. I think we should stick with the VRS Gene as much as possible (cf that one GH issue) and if we need to have other objects, call them something more specific like SourceGene or UngroupedGene.

@korikuzma
Copy link
Member

  1. Should this be converted to an epic?
  2. Can we rename DynamoDbDatabase to DynamoDb or DynamoDatabase? I think it was done to be consistent with child class names ending in Database, but seems redundant to have db and database in the name

@jsstevenson
Copy link
Member Author

Should this be converted to an epic?

Yeah, I think so -- but I do think we should plan on making as many of these changes as possible within a single discrete release to minimize headaches from updating

@jsstevenson
Copy link
Member Author

I think I've successfully broken out everything listed here into separate issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request priority:low Low priority
Projects
None yet
Development

No branches or pull requests

2 participants