monarch-initiative / monarch-legacy

Monarch web application and API
BSD 3-Clause "New" or "Revised" License
42 stars 37 forks source link

Add source images to golr pages #706

Closed kshefchek closed 9 years ago

kshefchek commented 9 years ago

Currently coded here, will refactor to use the handler and linker classes. The paths may be better placed in a config file, perhaps integrated with xrefs.json

kltm commented 9 years ago

This almost certainly should be in an external file that is read in as configuration--it should not be necessary to involve a programmer to modify images here. Also, is there a reason for regexps? (If worried about casing, you could always match lowercase.) Frankly, since you control all of the images locally, an easier pattern might just be to lowercase the id and use that in the image path (as standardize on a uniform image type).

Also, and this might be better in a new ticket, you want to not hand-generate JSON for you own consumption like this out of strings:

https://github.com/monarch-initiative/monarch-app/blob/0f6245e/lib/monarch/web/widgets.js#L1428

For example, if you had a label that contained quotes, things would get bad very fast. It's probably better to use the native JSON object or some such.

kshefchek commented 9 years ago

This code was written some time ago, I think this can be added to the xrefs.json and processed in the same way we process links @ktlm do you disagree with that approach?

kltm commented 9 years ago

That might be fine. What are you using for the upstream of the file? Is there a mini-pipeline for compiling it?

kshefchek commented 9 years ago

@kltm this is what I'm thinking: https://github.com/monarch-initiative/monarch-app/commit/ec3b8bd2c7500174cf571780e13bcd30233e5060

I've only updated this for Orphanet, but adding the rest to the xrefs.json will only take a few minutes

kltm commented 9 years ago

@kshefchek while it may be fine to put additional information in there, is this data is coming from upstream somewhere, that means that you'd have to re-add this data every time, essentially creating a mini pipeline. Is that in the cards here?

I think a "shared secret" might be just fine here, depending on what you metadata needs are (e.g. not handling a lot of ico and description information for resource yourself). Imagine:

'ORPHANET' comes in lowercase('ORPHANET') -> 'orphanet' by convention all icons are like 'blah/image/icon/X.png' so without a lookup you make the image link: blah/image/icon/orphanet.png

Again, depending on your data needs, it's very easy for non-programmers to maintain and requires no lookup. Downside, you handle the images, but it looks like you're doing that anyways.

kshefchek commented 9 years ago

I don't think we have a pipeline for getting these images. I don't think we update them often but maybe I'm misunderstanding your point. What would we be "re-adding" exactly? Having a naming convention for the images would also work.

kltm commented 9 years ago

Sorry, I'm going to try the Android client. The re-adding business is about the xrefs.json; if that is an upstream file that you're consuming, you'll have to keep merging your changes in.

kshefchek commented 9 years ago

We pull it in with pup-tent and turn it into a global variable (maybe not the best idea). I just replicated what you did with the golr conf json file.

kltm commented 9 years ago

Correct, but that was not a one time thing. In this case, GO data is upstream, and you'll have to have a way to remerge your changes - the file you're using is already out of date.

kshefchek commented 9 years ago

@kltm Let's chat about this next week. I might need some clarification on the best way to pull in these configuration files and managing this process. I think it's a little more involved than I realized.

kshefchek commented 9 years ago

Revisiting this morning, I think the "shared secret" option may not work as we have different file extensions for each image. In regards to the xrefs file, I noticed that it's been updated manually a handful of times since we brought it in from GO. So perhaps we will need to write a script anyways to merge any upstream changes with the monarch additions, see xrefs.conf history

cmungall commented 9 years ago

we have different file extensions for each image

so let's use the same format for all? keep it simple

kshefchek commented 9 years ago

@cmungall that is true and wouldn't take too much time. But regardless we need to be careful when regenerating this file from GO as we've made manual updates in this repo. I'm not too familiar with the process so maybe I'm misunderstanding things.

kltm commented 9 years ago

@kshefchek what kind of changes have been made? Looking at you xrefs.json file, it seems like is might have started from an earlier version of the GO.xrf_abbs file? Either way, you could just have a merge step (i.e. keep image or other information separate and join them at startup time), which would allow you to keep bringing in the new information automatically.

However, if the only extra information you need to add to this file is image paths, it's probably much easier to just get uniform image names by a bulk script (which makes other things easier, like uniform size, etc.) and get the path by an algorithm.

kshefchek commented 9 years ago

Going through the git history, I'm seeing 10-15 new xref entries that we have added. So in this case would we store these in a separate xrefs file and merge them at startup?

kltm commented 9 years ago

@kshefchek and these don't exist in the upstream version of the file? If not, it might be better to put in a pull request with upstream and get them merged at the source. If you don't know where this data comes from, we can poke @cmungall .

kshefchek commented 9 years ago

I can't find some of our additions in the yaml file in the amigo repo (ctd, dbvar, genereviews). So I can update the file there and put in a pull request.

kltm commented 9 years ago

@kshefchek AmiGO's upstream is here:

https://github.com/geneontology/go-site/blob/master/metadata/db-xrefs.yaml

I'd think that additional xref data would be fine there if you're consuming that.