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

Fix malformed JSON for collection ID #31

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@ianmilligan1
Copy link
Member

ianmilligan1 commented Mar 6, 2019

@SamFritz and I were doing some local testing on master and got the following error (on both of our systems):

screen shot 2019-03-06 at 4 28 53 pm

Somewhere along the line when the collection ID was added in, an escape went mixing which threw off the full cell. This branch works on both of our systems now.

screen shot 2019-03-06 at 4 29 49 pm

I'm not sure how this happened as it was definitely working locally earlier.

@greebie is currently working on a branch though. So maybe you just want to make sure that it works, so this is more just to have a branch that works (I have it at draft right now).

@ianmilligan1 ianmilligan1 requested a review from ruebot Mar 6, 2019

@ruebot

This comment has been minimized.

Copy link
Member

ruebot commented Mar 6, 2019

My bad.

@ruebot

ruebot approved these changes Mar 6, 2019

@ruebot

This comment has been minimized.

Copy link
Member

ruebot commented Mar 6, 2019

Let's get this one in now. It's an easy change for @greebie to pull into his work.

@ianmilligan1 ianmilligan1 marked this pull request as ready for review Mar 6, 2019

@ianmilligan1 ianmilligan1 requested a review from greebie Mar 6, 2019

@ianmilligan1

This comment has been minimized.

Copy link
Member Author

ianmilligan1 commented Mar 6, 2019

Ok I have marked this as ready for review. @greebie can you review + merge?

@ianmilligan1

This comment has been minimized.

Copy link
Member Author

ianmilligan1 commented Mar 7, 2019

Or actually, I guess @SamFritz can review too since she was in the room and verified that this works! 😄

@SamFritz

This comment has been minimized.

Copy link
Member

SamFritz commented Mar 7, 2019

I recloned the repo so I'll be able to do a walk through with a fresh slate. Will let you know if anything seems amiss.

@greebie

greebie approved these changes Mar 7, 2019

Copy link
Collaborator

greebie left a comment

This will work, but #28 already changes the name and structure of coll_id in the example file. If we want to store this commit so that the previous merge works fine, let's merge, otherwise we can just work with #28. Either way is fine with me.

@ruebot

This comment has been minimized.

Copy link
Member

ruebot commented Mar 7, 2019

Let's get #30 finished then.

@ruebot ruebot closed this Mar 7, 2019

@greebie

This comment has been minimized.

Copy link
Collaborator

greebie commented Mar 7, 2019

Almost there. Just fixing a few tiny bugs and we should be good to go in the next commit..

@ruebot ruebot deleted the json-fix branch Mar 7, 2019

@SamFritz

This comment has been minimized.

Copy link
Member

SamFritz commented Mar 7, 2019

this now works for me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.