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 rfc6901 implementation #3

Closed
wants to merge 2 commits into from
Closed

Conversation

gernest
Copy link

@gernest gernest commented Sep 5, 2019

RFC6901

This is used when referencing previous method results
https://tools.ietf.org/html/draft-ietf-jmap-core-17#section-3.6

OFFTOPIC : I just discovered this project today, I have been working on jmap go implementation too.I was more focused on the server, I see you focused more on the client. I will try go integrate my server code into this, I don't think its necessary to duplicate effort.

@foxcpp
Copy link
Owner

foxcpp commented Sep 5, 2019

https://travis-ci.com/foxcpp/go-jmap/jobs/231497768

json_ponter.go:48:10: v.MapRange undefined (type reflect.Value has no field or method MapRange) (typecheck)
		r := v.MapRange()
		       ^

This function was added in Go 1.12, Can you update .travisci.yml to use that version?

We can ignore failed build on Go 1.13, there is known bug in typecheck.

@foxcpp
Copy link
Owner

foxcpp commented Sep 5, 2019

OFFTOPIC : I just discovered this project today, I have been working on jmap go implementation too.I was more focused on the server, I see you focused more on the client. I will try go integrate my server code into this, I don't think its necessary to duplicate effort.

I really want to get JMAP support into maddy so that would be very nice. Created an issue for server implementation discussion (#4), you can also PM me on freenode IRC (foxcpp) if you want to talk about server design in general.

@gernest
Copy link
Author

gernest commented Sep 6, 2019

@foxcpp

This function was added in Go 1.12, Can you update .travisci.yml to use that version?

Done.

I really want to get JMAP support into maddy so that would be very nice. Created an issue for server implementation discussion (#4), you can also PM me on freenode IRC (foxcpp) if you want to talk about server design in general.

I am not familiar with maddy yet, but from the issue you linked its clear that go-jmap will be generic so backend specific details can be handled separately. I mean more like a library from which someone can assemble a functioning jmap server.

I'm not familiar with irc will try to chat after googling my way through it.

My current jmap efforts were more as a server and not a library for implementing jmap servers, I will try to pickup good parts and apply them to go-jmap

@foxcpp
Copy link
Owner

foxcpp commented Sep 6, 2019

Provided that this PR does not implement JMAPs extension to the JSON Pointer (* for mapping through array), why do you think it is necessary to roll your own implementation instead of using any of the other publicly available implementations?

Also, some information about original JSON object is lost when it is converted to a structure. Especially, if there is a custom UnmarshalJSON function or struct tags (we use a lot of them, but they doesn't seem to be supported by your code). I think it is necessary to operate on the JSON data model level (e.g. on map[string]interface{}) instead of already translated structure object.

Also, I have concerns about performance due to a heavy use of reflection in this PR. Since JSON pointer resolution is going to end up in the "hot" code for requests processing, this is important to us.

@gernest
Copy link
Author

gernest commented Sep 6, 2019

I see the project you linked meets your standards. I'm closing this in favour of the linked project.

I was using this to get familiar with your project.

@gernest gernest closed this Sep 6, 2019
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.

2 participants