-
Notifications
You must be signed in to change notification settings - Fork 80
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
fix #1693 #1742
fix #1693 #1742
Conversation
@@ -1011,6 +1011,10 @@ def _build_mapping_file(self, samples, rename_dup_samples=False): | |||
samps = set(samps) - all_ids | |||
all_ids.update(samps) | |||
|
|||
# appending the study title | |||
qm['qiita_study_title'] = qdb.artifact.Artifact( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Historically (I think) the name of the column has varied from title
to study_title
, etc, do we want to add yet another column name? I think the answer maybe is yes, because some studies have a legacy title
column that is inaccurate right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct. Basically, this way we make sure that legacy and new ones have it. Also the reason I prepended with qiita_
👍 looks good, just one small question that shouldn't be blocking. |
After going over the issues I realized that a few extra minor changes will also fix #1395 so decided to add them. Note that the delete of the prep template also had a small bug that is being fixed here. |
@@ -1011,6 +1011,18 @@ def _build_mapping_file(self, samples, rename_dup_samples=False): | |||
samps = set(samps) - all_ids | |||
all_ids.update(samps) | |||
|
|||
# appending study metadata to the analsis |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixing.
Two small comments |
qm['qiita_owner'] = '%s (%s)' % (study_owner.info['name'], | ||
study_owner.email) | ||
qm['qiita_principal_investigator'] = '%s (%s)' % ( | ||
pi.name, pi.email) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we include the email, then we are absolutely including the @
symbol, which is an unsupported character as per the QIIME mapping file description, I think this is fine with just the PI's name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent point, removing.
👍 ready for merge once tests pass. |
so delete is available? |
Yup, note that I just realized that it's blocking; will need to fix that. |
No description provided.