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

add as_json behaviour, closes issue #72 #73

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ConorSheehan1
Copy link
Contributor

@ConorSheehan1 ConorSheehan1 commented Oct 4, 2018

  1. alias as_json to to_ordered_hash to close issue as_json keys and key order different to to_json #72
  2. fix tests (princeton loris url changed, faraday not redirecting on 301)
    done on pr rspec princeton patch #74
  3. add shared_example to test as_json behaviour
  4. move shared_examples to spec/support to follow rspec convention
    https://relishapp.com/rspec/rspec-core/v/3-8/docs/example-groups/shared-examples

@coveralls
Copy link

coveralls commented Oct 4, 2018

Coverage Status

Coverage remained the same at ?% when pulling 3b76a54 on ConorSheehan1:as_json into d659b73 on iiif-prezi:development.

@jcoyne
Copy link
Collaborator

jcoyne commented Oct 4, 2018

@ConorSheehan1 this is a lot of stuff to digest all at once. Can you split out 7a3bd12 into it's own Pull Request?

@ConorSheehan1
Copy link
Contributor Author

@ConorSheehan1 this is a lot of stuff to digest all at once. Can you split out 7a3bd12 into it's own Pull Request?

@jcoyne no problem, should be on pr #74

@jcoyne
Copy link
Collaborator

jcoyne commented Oct 4, 2018

Thanks @ConorSheehan1. Can you rebase this on master now?

@ConorSheehan1
Copy link
Contributor Author

Thanks @ConorSheehan1. Can you rebase this on master now?

Hey sorry that took a while. Ran a merge from develop I shouldn't have. I think it's ok now

@jcoyne
Copy link
Collaborator

jcoyne commented Oct 4, 2018

The parent of c6c23bf is 1113f6b so it doesn't look like it's been rebased against master.

@ConorSheehan1
Copy link
Contributor Author

Sorry I've left the office already, I'll have to fix it tomorrow morning.

@ConorSheehan1
Copy link
Contributor Author

ConorSheehan1 commented Oct 5, 2018

Hey @jcoyne I think I'm a bit confused about what you want me to do.

1113f6b is the last commit on master,
so when I run git pull --rebase upstream master I'm getting already up to date.

I think the issue is I have changes here which are already applied on development in pr #74
I'm not really sure how to resolve that though.

I could undo that last commit c049588 which was a merge from development to get everything in sync. Then I'd only have the original changes I made to get as_json working. However that does include some changes to the specs so I think I'd have conflicts.

Edit: I could also just re-add the changes as a single commit, something like this?

What would you like me to do?

add shared_example for as_json to relevant specs
move shared examples to spec/support following rspec convention
https://relishapp.com/rspec/rspec-core/docs/example-groups/shared-examples
@jcoyne
Copy link
Collaborator

jcoyne commented Oct 5, 2018

I don't think that's right. d659b73 is the latest commit on master. I don't know why git pull isn't fetching the latest stuff for you.

@ConorSheehan1
Copy link
Contributor Author

I don't think that's right. d659b73 is the latest commit on master. I don't know why git pull isn't fetching the latest stuff for you.

Ah ok I think I see where the confusion came from. d659b73 is on development, not master. I've just added the changes again as a new commit with d659b73 as the parent. Is that what you were looking for?

@jcoyne
Copy link
Collaborator

jcoyne commented Oct 6, 2018

Ugh, yes, sorry, I didn't realize that development was the default branch on this project.

@minusdavid
Copy link

I take it this one is closed now?

@ConorSheehan1
Copy link
Contributor Author

ConorSheehan1 commented Sep 10, 2019

I take it this one is closed now?

The fix for the tests was split out into pr #74 and merged, but this is still open. Currently obj.as_json and JSON.parse(obj.to_json) return different results, which is quite unexpected. This pr would fix that.

@minusdavid
Copy link

I suppose I'm curious since this project hasn't had any new commits since October 2018.

I'm wondering if the project is dead now, or if @jcoyne is still working on it.

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.

4 participants