Closed chaoran-chen closed 2 years ago
This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.
🔍 Inspect: https://vercel.com/hodcroftlab/covariants/5LBbDCvoGPqbpXGPvzMby6xBoiAc
✅ Preview: https://covariants-git-fork-chaoran-chen-patch-1-hodcroftlab.vercel.app
Thanks for spotting this @chaoran-chen ! I hadn't noticed this little bug. 🙈 @ivan-aksamentov if this looks good to you, I'm happy to merge.
@emmahodcroft Looks okay. But the main reason for it to happen is the empty strings for alt names in the data. They should not be there (can be None or omitted, I think). Feel free to either:
@chaoran-chen Ah, sorry, I confused something. I meant empty arrays, not empty strings.
The Typescript definition is
alt_display_name?: string[]
which means "missing or array of strings".
So then I wrote the check in the component
cluster?.alt_display_name
I assumed that lack of alt name will be expressed as a missing field, but in the data it seems to be expressed as an empty array. Just a little miscommunicaton between me and Emma.
Ah ha - I see what you mean Ivan. I would be happy to do 3 - both. I think it's nice if we can handle things like accidental empty arrays neatly (not unimaginable it happens again), but still do this kind of tidy-up.
I'll try to make this change to tidy up these 'empties' in clusters.py
on master.
I'm just letting that build now to check that I didn't manage to break anything, but if that looks good, and Chaoran's changes look good, feel free to merge this Ivan!
Edit:
Unless I'm having a caching problem (possible, the internet here isn't great), removing these empty arrays from clusters.py
didn't seem to solve the underlying problem on master. Might want to merge master into this branch to ensure having these missing completely doesn't break Chaoran's code and still fixes the problem!
@emmahodcroft
removing these empty arrays from clusters.py didn't seem to solve the underlying problem on master
Web data needs to be updated to regenerate web/data/clusters.json
I am merging this PR, because it's seems to be low risk.
@chaoran-chen Could you please add yourself to the list of contributors as explained here: https://github.com/hodcroftlab/covariants/pull/48 (and also in our other projects where you contributed) 🙏
Ah, of course, sorry Ivan for the oversight - and thank you!
This hides the "also known as" text in the variant title if there is no alternative name.
How it is right now: