Skip to content
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

Trackhubs #6

Open
wants to merge 25 commits into
base: master
Choose a base branch
from
Open

Trackhubs #6

wants to merge 25 commits into from

Conversation

alanfwilliams
Copy link
Contributor

This pull request adds functionality for trackhubs in SegAnnDB.

… or lists, fixed typos in trackhub_file and trackhub_list, added tests for trackhubs and headers and made tests.py indentation consistent, simplified utilities install by using precompiled binaries.
…lowed trackhub creation of all avaliable data, amended test file to account for new file names and fixed typo
…s were added to trackDb.txt, added more descriptive info for the track hub output
</form>

<form method="get" action="/trackhub_init/${user}/">
<button type="submit">Create Track Hub</button>
Copy link
Owner

@tdhock tdhock Aug 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry for not mentioning this earlier but it is usually a bad idea to use a form if there are no fields/data to submit, as is the case above. in this case you could just use a standard link, a href instead, right?

and actualy i would suggest changing the URL scheme from /trackhub_*/${user}/ to simply /trackhub_*/ because anyways the user variable is stored in the session info

plotter/views.py Outdated
if linelist == ['']:
continue
if len(linelist) >= 2:
if linelist[0][3:] not in cl.keys():
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add comments to clarify you intention in this new block of code.. it is not clear to me what you are doing here

plotter/views.py Outdated
'\nshortLabel ' + request.POST['short_label'] + datatype + '\nlongLabel ' +
request.POST['long_label'] + datatype + '\ntype bigBed\ncolor 0,253,0' +
'\nalwaysZero on\n\n')
trackdbtxt.write('track ' + x + '\nbigDataUrl ' + x + '.bigWig\nshortLabel ' + request.POST['short_label'] +
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there some way to remove repetition in these trackdbtxt.write lines?

also instead of using 'track ' + x + "_" you may consider using 'track %s_'.format(x) etc

tests/tests.py Outdated
submit_button.click()
sleep(10)
if ("Trackhub data for test1" in driver.page_source):
assert True
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice, glad to see that you added a test for the track hub functionality that you implemented

you may also want to check for the presence of the profile name, ES0004, on the web page

also it would be good test track hubs with multiple profiles (here there is only one profile in the track hub, right?)

@tdhock
Copy link
Owner

tdhock commented Aug 10, 2018

thanks for the code contributions! nice to see that tests are passing. please address my comments the best you can and then we can merge with master

…ents, userid usage, string formatting, and bug fix in multiple profile creation (with accompanying test) to trackhub code
@alanfwilliams
Copy link
Contributor Author

I did correct everything you asked for except for removing the repetition. There was a lot of repetition (more than currently) to begin with, and the current loops are about as small as I can make it right now.

plotter/views.py Outdated
for x in request.POST.getall('profile'):
trackdbtxt.write('track %s' % x + 'multiWig\ntype bigWig\ncontainer multiWig\naggregate transparentOverlay\n' +
'shortLabel ' + request.POST['short_label'] + 'multiWig\nlongLabel ' +
request.POST['long_label'] + 'multiWig\nautoScale on\nnegativeValues on\nalwaysZero on\n\n')
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry for not being clear earlier -- I meant to say that it would be good to replace all of the + with % (not just the first)

In fact I think it would be even easier to understand/read if you use triple quotes and % here, e.g.

    trackdbtxt.write("""track %smultiWig
type bigWig
shortLabel %smultiWig
"""%(x, request.POST['short_label'])

here i just show what it should look like with two variable substitutions in the text, but please use %s/etc (not +) for all variable substitutions

the only difference between single and triple quotes is that triple quotes allow line breaks in literal character strings... which is very useful from a readability perspective here

@alanfwilliams
Copy link
Contributor Author

Thanks for the clarification/suggestion. Looks much better now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants