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

Go templates update #353

Merged
merged 198 commits into from
Apr 15, 2022
Merged

Go templates update #353

merged 198 commits into from
Apr 15, 2022

Conversation

phaus
Copy link
Contributor

@phaus phaus commented Jan 29, 2022

Follow PR of #287

Copy link
Member

@lohanidamodar lohanidamodar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really promising. Just few concerns.

templates/go/client.go.twig Outdated Show resolved Hide resolved
templates/go/client.go.twig Outdated Show resolved Hide resolved
templates/go/go.mod.twig Outdated Show resolved Hide resolved
tests/SDKTest.php Outdated Show resolved Hide resolved
tests/languages/go/tests.go Show resolved Hide resolved
Copy link
Contributor

@abnegate abnegate left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work so far 😁

example.php Outdated Show resolved Hide resolved
src/SDK/Language/Go.php Outdated Show resolved Hide resolved
src/SDK/Language/Go.php Outdated Show resolved Hide resolved
src/SDK/Language/Go.php Show resolved Hide resolved
templates/go/README.md.twig Outdated Show resolved Hide resolved
templates/go/docs/example.md.twig Outdated Show resolved Hide resolved
templates/go/go.mod.twig Show resolved Hide resolved
templates/go/main.go.twig Outdated Show resolved Hide resolved
templates/go/docs/example.md.twig Outdated Show resolved Hide resolved
tests/SDKTest.php Outdated Show resolved Hide resolved
@phaus
Copy link
Contributor Author

phaus commented Feb 2, 2022

Hi @christyjacob4! I did react to all suggestions. Is there anything else, I can do?

tests/SDKTest.php Outdated Show resolved Hide resolved
Copy link
Member

@lohanidamodar lohanidamodar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some more small changes.

templates/go/README.md.twig Outdated Show resolved Hide resolved
templates/go/README.md.twig Outdated Show resolved Hide resolved
templates/go/README.md.twig Outdated Show resolved Hide resolved
templates/go/client.go.twig Outdated Show resolved Hide resolved
templates/go/main.go.twig Outdated Show resolved Hide resolved
templates/go/main.go.twig Outdated Show resolved Hide resolved
tests/SDKTest.php Outdated Show resolved Hide resolved
Copy link
Member

@lohanidamodar lohanidamodar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more small request.

Also can you generate and push the generated go sdk in your own separate repository and provide a link want to look at the final code.

templates/go/client.go.twig Outdated Show resolved Hide resolved
@phaus
Copy link
Contributor Author

phaus commented Feb 10, 2022

@lohanidamodar I did update the go-sdk repo with the latest generate code.
You can find it here: https://github.com/phaus/sdk-for-go

@lohanidamodar
Copy link
Member

@phaus Saw the generated code, looks awesome. I have a question
Is something like data objects common in Go or we just expose the JSON we receive from the server?

@phaus
Copy link
Contributor Author

phaus commented Feb 15, 2022

@lohanidamodar

At the moment a client call returns a map with all elements as key => vallues.
One could migrate that to specific structs to make the API more specific.
For that reason I would need an overview about the fields that are used. Is there a deploment a swagger UI for the provided swagger spec somewhere?

@lohanidamodar
Copy link
Member

@phaus I don't remember that we have a swagger UI, however, if you look at our existing SDKs (Dart, Android, Kotlin, Apple, Swiift, all of these have response model implemented). The details of definitions can be found in our spec as well under definitions key. Look at the existing SDKs and please let us know if there's any confusion.

@lohanidamodar
Copy link
Member

Also, I would love you to sync this PR with latest master, we have restructured testing to make it more effective, let us know if there's any confusion.

@phaus
Copy link
Contributor Author

phaus commented Feb 16, 2022

@lohanidamodar I did a rebase to the current upstream master branch.
However, to have valid go code generated, the sdk env GitUserName has to be appwrite.
Since that seems to impact all other tests as well, I need some hints on how to impliment it in a better way.

I will have a look into response struct the next days.

@phaus phaus force-pushed the go-templates-update branch 2 times, most recently from a4b8831 to a246a97 Compare February 16, 2022 23:46
@phaus
Copy link
Contributor Author

phaus commented Feb 17, 2022

TBH I have actually no idea why the Flutter Test is failing right now

@lohanidamodar
Copy link
Member

@phaus Why do you need gitusername to be appwrite, you are not actually pulling from github for test right, you are using the local instance. In the testfile can you can make appwrite to githubuser or something that was perviously there? Modify the test.

@lohanidamodar
Copy link
Member

TBH I have actually no idea why the Flutter Test is failing right now

Don't worry about the Flutter test, that has nothing to do with what you did. Fix for that is already pending in my PR #372

@phaus
Copy link
Contributor Author

phaus commented Mar 23, 2022

I did the rebase, updated my test to the new setup.

@phaus
Copy link
Contributor Author

phaus commented Mar 31, 2022

deactivate the failing test case. Is there anything else, that is missing in this PR?

Copy link
Member

@lohanidamodar lohanidamodar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few more things to take care of before we can proceed. We want to get this right. Let us know if you need any help with chunked and resume-able upload

.gitignore Outdated Show resolved Hide resolved
templates/go/client.go.twig Outdated Show resolved Hide resolved
return false
}

func (clt *Client) newfileUploadRequest(uri string, params map[string]interface{}, paramName string) (*http.Request, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, we need a chunked and resume-able upload support here (check dart/web sdk where we have it already implemented)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is then something I have to implement later on.
Due to time constraints I won't be able to do that right now.
Should I disable the API then for now?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@phaus no we don't need to disable the API, if it's okay with you, we can proceed with getting this merged and add the chunked upload/resumable upload later by our team or you if you get time. If that's good and some other team member review it, we can proceed forward. We are really happy with the work you have put in and the patience you had with us. Just a little more and we can get this merged 🙏🏻

@lohanidamodar
Copy link
Member

@phaus I resolved the conflict, and re-enabled the disabled test. If everything goes well, we will get this merged.

Copy link
Contributor

@abnegate abnegate left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking great! Thanks for all your hard work 🚀

@lohanidamodar lohanidamodar merged commit 4a3cf9a into appwrite:master Apr 15, 2022
@phaus phaus deleted the go-templates-update branch April 19, 2022 14:44
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

3 participants