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

Ebi improvements pet #1490

Merged

Conversation

josenavas
Copy link
Contributor

Fixes the interface to show that EBI data in the corresponding spot.

@josenavas
Copy link
Contributor Author

Depends on #1489 so review/merge that one first


@property
def is_submitted_to_ebi(self):
"""Gets if the prep template has been submitted to EBI or not
Copy link
Contributor

Choose a reason for hiding this comment

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

This is minor - could you rephrase this from "Gets" to "Inquires"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@mortonjt
Copy link
Contributor

mortonjt commented Oct 8, 2015

Don't forget the pep8 error

experiment_alias = self._get_experiment_alias(sample_name)
sample_prep = dict(self.samples_prep[sample_name])
if self._ebi_sample_accessions[sample_name]:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to pass in samples that aren't contained in self._ebi_sample_accessions? If so, would it be worthwhile to add a check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It shouldn't. samples is a subset of the samples in the sample template, and _ebi_sample_accessions has an entry for each of the samples in the sample template.

@josenavas
Copy link
Contributor Author

Comments addressed.

@@ -39,6 +39,9 @@
'public':
('glyphicon glyphicon-eye-open', 'glyphicon glyphicon-globe', 'green')}

ebi_linkifier = ('<a href="http://www.ebi.ac.uk/ena/data/view/{0}" '
Copy link
Member

Choose a reason for hiding this comment

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

ebi_linkifier -> EBI_LINKIFIER

@ElDeveloper
Copy link
Member

Just one comment, code looks good otherwise. I'll take a pass through the interface though. @mortonjt, did you review the interface?

@mortonjt
Copy link
Contributor

mortonjt commented Oct 8, 2015

Nope. I'll try to take a look at the interface later today.

Jamie

On Thu, Oct 8, 2015 at 9:54 AM, Yoshiki Vázquez Baeza <
notifications@github.com> wrote:

Just one comment, code looks good otherwise. I'll take a pass through the
interface though. @mortonjt https://github.com/mortonjt, did you review
the interface?


Reply to this email directly or view it on GitHub
#1490 (comment).

@ElDeveloper
Copy link
Member

Thanks @josenavas , 👍 @mortonjt you can merge as soon as you are done testing.

@ElDeveloper ElDeveloper mentioned this pull request Oct 9, 2015
@mortonjt
Copy link
Contributor

mortonjt commented Oct 9, 2015

👍

ElDeveloper added a commit that referenced this pull request Oct 9, 2015
@ElDeveloper ElDeveloper merged commit e261564 into qiita-spots:ebi-improvements Oct 9, 2015
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.

None yet

3 participants