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

Datetime support for msgpack for issue-21721 #22118

Merged
merged 1 commit into from
Mar 30, 2015

Conversation

surajssd
Copy link
Contributor

datetime.datetime support added. In this PR only encoding code is added working on decoding part as well.
#21721

@thatch45
Copy link
Contributor

I like this idea but I think that it should be implemented in a more robust way. @cachedout and myself are going to talk it over and try to figure out how to make this viable. It poses a swath of security concerns and if we insert it like this it makes scaling issues and portability issues.

So in summary, this is a killer idea and I think we can implement it in a much more powerful way

@cachedout
Copy link
Contributor

@surajssd We sat down and talked about this and I think what we're going to do here is merge this and then immediately refactor it into an external library just to keep the serialization code as tidy as possible. Thanks for your contribution here, it should really help!

cachedout pushed a commit that referenced this pull request Mar 30, 2015
Datetime support for msgpack for issue-21721
@cachedout cachedout merged commit adb50ca into saltstack:develop Mar 30, 2015
@surajssd
Copy link
Contributor Author

surajssd commented Apr 1, 2015

@thatch45 and @cachedout yeah writing a seperate library that handles all msgpack issues would be better and could have more additions in near future if some where it fails again. And library supports easy encoding and decoding of datatypes into msgpack. Thanks for the merge.

@jfindlay
Copy link
Contributor

jfindlay commented Apr 1, 2015

@thatch45, @cachedout, also see #11630.

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