-
Notifications
You must be signed in to change notification settings - Fork 0
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
Release/v.1.0.0 #17
Release/v.1.0.0 #17
Conversation
…every field has a value), patient_accession_no -> dn_no
updates progressbar to the new requirements.txt
- 1 place where patient_accession_no -> dn_no was missed - 1 place where protein_(pnomen) -> pnomen was missed - /aaa was changed to /all - etc
add some explanation about datatables.js, as well as some more info about application structure
Bumps [jinja2](https://github.com/pallets/jinja) from 2.11.2 to 2.11.3. - [Release notes](https://github.com/pallets/jinja/releases) - [Changelog](https://github.com/pallets/jinja/blob/master/CHANGES.rst) - [Commits](pallets/jinja@2.11.2...2.11.3) Signed-off-by: dependabot[bot] <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to see this pull-request towards the first release of the VUSualizer, great!
I have a few general remarks.
external plugins to vendor
Robert explained to me that the vendor folder is often used for external software / plugins and should be ignored when reviewing the code.
He pointed out that src/static/js/jquery-3.4.1.min.js
is also part of src/static/vendor/jquery/jquery-3.4.1.min.js
.
It would help if all code developed by you/us/our team is part of static/js or static/css and all external code should go to static/vendor.
- Could you have a look into the src/static/js and src/static/css folders and move files when it concerns external software?
import_data.py
Based on previous pull-requests, I get the idea that this script has grown over time quite a bit.
- Personally, I would prefer to see more bite sized functions, and using
return
more often in import_data.py
For example:
- retrieve_info_analysis_alissa
- check_valid_analysis
- retrieve_vus_gus_analysis
- upload_to_mongodb
naming conventions
Some variables use a very generic and meaningless naming convention.
- Maybe you could go over the code changes and consider if/where variable names can be improved?
CSS files
Some of the .css files are huge and I am worried about sustainability as well as readibility.
I would suggest to have a look into CSS coding style guides, for example https://google.github.io/styleguide/htmlcssguide.html
There are a few CSS lint extensions available. Please use these as well.
I know we discussed CSS previously and decided it was out of scope and to postpone it to next release.
I think it would improve a lot if you could add comments of the different sections/group of code in each .css file, for example src/static/css/sb-admin-2.css could have more comments.
- Add comments to custom .css files describing code blocks.
Remaining notes
- For each html in src/templates, I see a warning message: "No newline at end of file". Could you perform some HTML lint checking and see whether newline should be there?
- I added a few comments related to flake8 output. Might be good to rerun flake8 just before merge to master branch.
- [ ]
I think I addressed most of your points. I await your next review and possible comments on my reply's
|
…t mongo key, and moving files to vendor
… disclaimer added
…ge and gene/patient/variant page datatable
We talked on teams about how to tackle these two functions in datatable.py. We agreed upon to make it one function. While trying to merge this (without having specific Mongo entries require to query them using keys (and then appropriate extra quering if that key has multiple values in a dict or list) Could you have another look at this proposed solution with extra comments and explanation? of course I would like to hear it if you see this differently. |
…based on software validation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small changes, all approved
Release first production version (V1.0.0)
Unverified commits:
Verified commits:
Login functionality #6 + #8
Cleanup layout #9
Alissa connection instead of using importing Excels from "O-schijf" #10
Layout changes to accommodate for the new Alissa method (replacing the Excel upload method) #13
Added restrictions for specific roles and updated account page #15