search

informatics-isi-edu / mesh-viewer

3D mesh viewer
1 stars 1 forks source link

change param and json properties #50

Closed robes closed 5 years ago

robes commented 5 years ago

I've documented a revised specification of the url params and json properties. These should just be name changes as I don't believe I've changed anything about the logic of how they are to be interpreted. I.e., label -> label_alt -> basename(url) for the mesh labels should be the same logic as anatomy -> label -> basename(url) is now.

I started a branch param_rename that you should use. And then refer to the README in that branch for the naming that you should conform the code to.

https://github.com/informatics-isi-edu/mesh-viewer/blob/param_rename/README.md

Please make your PR to this branch.

robes commented 5 years ago

@carlkesselman in case you have an opinion on this too. See the README that I link to above. This should be in line with your earlier sentiment to rename anatomy to label so that its more generic. I preserved a label_alt so that we dont have to make model changes in facebase (the label_alt becomes the fallback if label is not defined).

NickolausDS commented 5 years ago

RID is another exception as the only capitalized entity. Should that be lower cased?

robes commented 5 years ago

Actually, the right thing to do here is to make all of the json params conform to a Title_Case_With_Underscore style which is our preferred naming style for db model elements. I should just do that now while we are at it.

NickolausDS commented 5 years ago

One more thing, would it be possible to combine Label and Label_Alt?

robes commented 5 years ago

I don't think so.

NickolausDS commented 5 years ago

I think that's everything. Do you have a model setup with these spec changes to test with?

NickolausDS commented 5 years ago

Also, should the URL params change to Model, Mesh, and Landmark?

robes commented 5 years ago

It will work with our existing model data, we just need to change the url to match the revised param and property names.

No, the URL params can remain lower case.