PatrickWeller / PanelPal

1 stars 0 forks source link

Issue68 - Improve the quality of the Panel App API Functions script #69

Open PatrickWeller opened 6 days ago

PatrickWeller commented 6 days ago

For #18 and Sprint 3 I've brought the panel app api functions up to a higher quality

  1. Improved Error handling
  2. PEP8 compliant according to pylint image
  3. In line commenting
  4. Doc strings in Numpy format
  5. Top level documentation in Numpy format
  6. Logging added to key bits of commands, and errors. I know we said we wouldn't for accessories files, but this seems OK to me after considering it, very few logging lines will be run from these commands anyway.
  7. Created tests with 100% pass rate: image
  8. Created tests with 100% coverage for panel_app_api_functions.py according to "coverage report -m" image

I also added an additional function that will contribute towards #52 #57 (Duplicate issue) to be able to compare gene lists for user story #56 . The function is:

The above required using the database primary key for the panel, rather than the R number, so this was also added as an output of get_genes

Creation of a logger is temporarily in this file to be removed once I tackle loggers more fully. README.md not added to. Not sure if it will need to go in there at all, but for now waiting for Ash to redo the README.

madysonic commented 2 days ago

Documentation looks good, and good amount of tests implemented. Tests all pass on my machine too. As discussed, you have added the time out variable to api requests, but we need to add additional handling for when it times out and write tests for this.

Also, do you think it is likely we will need to use panel_pk instead of panel_id (R number) for more than just comparing gene lists? If so we may need a function in the future to easily convert between the two. Just a thought.

PatrickWeller commented 1 day ago

Thanks Mady, I will add those tests and handling today.

Good thought about the R number and panel primary keys. The get_name_version function does id > pk but yes, a pk > id function might be a good idea should we want to shove that into bigger scripts. Though I can't exactly think of a use case right now. As users at least will likely only have the R number. So I'm thinking that I'll add it as it's own issue, but it won't be a priority until we see an explicit reason for needing it. It won't be a lot of work if we need it. But we don't want it to become a repeat of giving Jenkins a 9th eye and a scorpion tail; additional unnecessary work.