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

Drop fatih/structs #90

Open
ferhatelmas opened this issue Jun 18, 2020 · 11 comments
Open

Drop fatih/structs #90

ferhatelmas opened this issue Jun 18, 2020 · 11 comments

Comments

@ferhatelmas
Copy link
Contributor

ferhatelmas commented Jun 18, 2020

It's not supported anymore, good to refactor its usage and drop.

see #85 #89 for an issue caused by it in nested structs.

@nchetan-zz
Copy link

@ferhatelmas I am looking to start contribution to go-lang projects right now. Since this is marked as "good first issue", I plan to take a stab at it.

@ferhatelmas
Copy link
Contributor Author

@nchetan awesome, idea is to drop dependency and implement the existing functionality with standard library (reflect) directly. Go ahead!

@nchetan-zz
Copy link

Do you have documentation on how to build the code base and how to execute it? The README is very user centric and not contribution centric. Please help (I will try and figure this out till you answer back. Will leave a note if am successful)

@ferhatelmas
Copy link
Contributor Author

ferhatelmas commented Jun 22, 2020

@nchetan I will add one today: #92

In the meantime, were you able to make any progress?

@nchetan-zz
Copy link

Yes. I have.. Is there a way to sync on slack or another software instead of conversations here? I can be reached on email, slack, phone, skype, FB messenger etc.
Is there a specific one that you prefer to reach out?

I am based in Pacific Standard Time.

@nchetan-zz
Copy link

nchetan-zz commented Jun 22, 2020

@ferhatelmas Also, to see an alternative approach, check if https://play.golang.org/p/1sJ3YT7pPiA works for you. This approach doesn't use reflect. If the approach does not work for any reason, please let me know what is missing?
Please use this as a suggestion and let me know if it's better.
If not, reflect it is....

@nchetan-zz
Copy link

nchetan-zz commented Jun 22, 2020

For comparison, I also created https://play.golang.org/p/0KeJ4rej1p9 (Note: This doesn't create a finished JSON. Just a showcase).
Let me know if the alternative approach is good.

@nchetan-zz
Copy link

@ferhatelmas I am still waiting for a response. Thoughts?
If I don't hear back from you, I am going to start working on the alternative approach and send a merge request. That way, you get a chance to look at the code and try it out as well.
Will wait till 16:00 (4PM) GMT time on 6/24/2020 to hear back from you (before starting on the approach I mentioned). In the worst case, I create a solution that you don't like and don't accept.

@nchetan-zz
Copy link

@ferhatelmas If I don't hear back from you in next couple of days, I will assume that this is not a priority issue and drop this issue and continue looking for other places to contribute.

@nchetan-zz
Copy link

Here's the update code right now. @ferhatelmas
https://play.golang.org/p/cRVlkReyFdj

@ferhatelmas
Copy link
Contributor Author

@nchetan Yes, it's not a priority. However, we can work on it slowly in that direction.

Regarding play links, they are fine for a quick show but a PR would be better for a bigger change.

Moreover, there are issues in the implementation and idea because we want to use json.Marshal and json.Unmarshal and they should do the right thing automatically. Implementation should support similar recursive structs (activity contains reactions etc.) Finally, they are marshaled into json, we keep them in the result but how they are put into structs in the memory is different than default because they are optional.

I have a similar PR here: GetStream/stream-chat-go#84
You can give a look at the idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants