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

Issue 32 #33

Merged
merged 11 commits into from Aug 18, 2016
Merged

Issue 32 #33

merged 11 commits into from Aug 18, 2016

Conversation

kmckinnis
Copy link
Contributor

I switched around the order of the output to help fix --existing ids and changed the label maker to help make sure that it prints the cual-id after the first change was applied.

… of different templates for barcode creation. It adds on the tag --small with label creation that creates a 5X17 grid that has not been tested yet, but once it has, will fit this sheet http://www.sigmaaldrich.com/catalog/product/sigma/z688363?lang=en&region=US&cm_sp=Insite-_-prodRecCold_xviews-_-prodRecCold10-5. Also added a couple of methods to avoid repetition as per DRY standards.
…l-ids, it can still comfortably fit on the .94x.5 inch labels. Still not imperically tested yet.
…ing the cual-id printed first, in order to make the duplicate checking method working. Also edited the label maker so that it would work with the edits made.
@coveralls
Copy link

Coverage Status

Coverage decreased (-10.07%) to 83.333% when pulling d386495 on kmckinnis:master into da1ca2c on johnchase:master.

@johnchase
Copy link
Owner

@kmckinnis will you remove the changes that address #2 and only submit changes that address #32? I will review then.

@kmckinnis
Copy link
Contributor Author

Alright, I will try to do that, but I am busy right now on a project from
Nicole. Could get it done late today or early tomorrow.

On Tue, Aug 16, 2016 at 2:37 PM, John Chase notifications@github.com
wrote:

@kmckinnis https://github.com/kmckinnis will you remove the changes
that address #2 #2 and only
submit changes that address #32
#32? I will review then.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#33 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ATEw0YHgrxc_-WU_MsdaDufgDS_EJ_qIks5qgi2TgaJpZM4Jl2ID
.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 93.407% when pulling b6a9e5f on kmckinnis:master into da1ca2c on johnchase:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 93.407% when pulling 5d9cb7a on kmckinnis:master into da1ca2c on johnchase:master.

@@ -26,7 +26,7 @@ def get_barcodes(input,
x_start=1.9,
y_start=257.2):

ids = [e.strip().split('\t')[1] for e in input]
ids = [e.strip().split('\t')[0] for e in input]
Copy link
Owner

@johnchase johnchase Aug 16, 2016

Choose a reason for hiding this comment

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

Sorry, looking at this closer, I think this change is correct, once the other changes to the code have been made

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 originally checked the second column where the cual-id was, but I
changed it to read the first. I think you may have read the change wrong.

On Tue, Aug 16, 2016 at 3:09 PM, John Chase notifications@github.com
wrote:

In cualid/label.py
#33 (comment):

@@ -26,7 +26,7 @@ def get_barcodes(input,
x_start=1.9,
y_start=257.2):

  • ids = [e.strip().split('\t')[1] for e in input]
  • ids = [e.strip().split('\t')[0] for e in input]

This will now check the existing IDs against the second column of whatever
file is passed. I thikn what we want to do is still check the first column,
but when the IDs are created have the cual-id in the first column and the
UUID in the second


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/johnchase/cual-id/pull/33/files/5d9cb7aafea8d965f1a16e20ea318ef05356b1b1#r75030212,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ATEw0SGvdnwDJrsz4fv1Ij5oKURnpGt5ks5qgjUYgaJpZM4Jl2ID
.

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, I agree, however this change only make sense in the context of other changes, which need to be added to this pull request

@coveralls
Copy link

Coverage Status

Coverage remained the same at 93.407% when pulling 3f4ee30 on kmckinnis:master into da1ca2c on johnchase:master.

@@ -69,4 +69,4 @@ def correct_ids(correct_ids, ids_to_be_corrected, show, all):


if __name__ == '__main__':
cli()
cli()
Copy link
Owner

Choose a reason for hiding this comment

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

Can you add the newline character back into this file?

@johnchase
Copy link
Owner

@kmckinnis This looks good so far, can you also update the fix.py script to compare the first column?
Can you also update the changelog?

@coveralls
Copy link

Coverage Status

Coverage remained the same at 93.407% when pulling 15cef93 on kmckinnis:master into da1ca2c on johnchase:master.

@johnchase
Copy link
Owner

@kmckinnis This all looks good, will you address my comment above about the newline character? It should be good after that.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 93.407% when pulling 05f0c34 on kmckinnis:master into da1ca2c on johnchase:master.

@johnchase johnchase merged commit 1bb93ae into johnchase:master Aug 18, 2016
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