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

move DefaultSerializer to a separate file, add tests #335

Closed
wants to merge 1 commit into from

Conversation

teeparham
Copy link
Contributor

Nothing was changed in DefaultSerializer. It was simply moved to a separate file.

@coveralls
Copy link

Coverage Status

Coverage increased (+0%) when pulling c96095f on teeparham:default_serializer into 4f185ef on rails-api:master.

@mikegee
Copy link
Contributor

mikegee commented Sep 9, 2013

I thought I saw something about work on a big rewrite, but it would be nice if someone could comment here on why this isn't being merged right now.

@steveklabnik
Copy link
Contributor

Yup.

@teeparham
Copy link
Contributor Author

@steveklabnik @spastorino

If there is a large rewrite happening with AMS, there should be a note in the Readme or an open Issue that says something like "we are only accepting bug fixes over the next [2 months/whenever], as we're re-writing the gem." It is discouraging to contributors to think that their valid work would be ignored. In any case, the communication could be handled better.

🌈🌈🌈

@steveklabnik
Copy link
Contributor

This is one of those things where it's supposed to take a week, then takes longer, then takes longer... but yes, hindsight is 20/20.

@spastorino
Copy link
Contributor

@teeparham neither your work or work done by other contributors will be ignored. I jumped here to help because lack of maintainers in the project. Will try to finish my work ASAP and then I'm going to take care of all the open issues and PRs.

@sorentwo
Copy link

@spastorino, @steveklabnik Thanks for getting back on top of communication. This is a project that is completely integral to all of the Rails work that I do and I appreciate knowing where it stands. I'm sure many of the other PR contributors feel the same.

Happy to help triage, test, advise where I can.

@teeparham
Copy link
Contributor Author

@spastorino @steveklabnik To be clear, I merely wanted to make a friendly suggestion to keep commits small & frequent & to try to keep communication open for work in progress. Thanks to both of you for stepping up to be maintainers. I'm happy to continue to help & have obviously been a fan & contributor for a while.

@spastorino
Copy link
Contributor

@teeparham thanks for helping, the delay is just my fault because I have a lot of stuff to do and couldn't finish it yet

@ghost
Copy link

ghost commented Sep 11, 2013

Thank you @spastorino for your amazing work. 👍

@tjschuck
Copy link
Contributor

@spastorino Any chance you could make this development a bit more public? Even just pushing up a branch that you're working on so we're not all left in the dark and where we can potentially provide our own feedback (and keep our own pull requests up-to-date with it).

It would be nice if this rewrite was carried out a bit more like the also-significant rewrite of ember-data was, where things were still done publicly in their jj-abrams branch so invested parties weren't blindsided by major changes.

I definitely appreciate all of your awesome work, and realize that life and "real work" can easily trump working on OSS, but it seems like you have quite a few people on here who would be willing to help out :)

❤️ ❤️ ❤️

@spastorino
Copy link
Contributor

@tjschuck will do as soon as I can. Just need to polish some stuff.

@spastorino
Copy link
Contributor

This is already in a different file in master. Thanks a lot for helping and sorry about the long delay. ❤️ ❤️ ❤️

@spastorino spastorino closed this Oct 21, 2013
@teeparham teeparham deleted the default_serializer branch October 21, 2013 20:09
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

7 participants