Skip to content

Conversation

@mrhdias
Copy link
Contributor

@mrhdias mrhdias commented Jul 9, 2023

Examples rely on kolo/xmlrpc as well as on the standard library package log to panic on error, matching snippets in other languages (which tend to raise exceptions).

As with other languages, only the RPC interaction is spelled out.

The code examples in Go have been added in the same places as existing examples for other programming languages. All Go examples are based on existing examples, but respecting the specificities of the Go language. All examples have been tested with a local installation of odoo v16 and work fine, except the odoo test database which always returns an error. The Go code for testing can be found here:
https://gist.github.com/mrhdias/f58106caa1e148c925cbe0b24635ef75
@robodoo
Copy link
Collaborator

robodoo commented Jul 9, 2023

@C3POdoo C3POdoo requested a review from a team July 9, 2023 16:21
mrhdias added 2 commits July 9, 2023 17:39
The RST text formatting errors have been fixed
The last issue with text formatting has been fixed.
@AntoineVDV AntoineVDV requested review from a team, ryv-odoo and xmo-odoo and removed request for a team July 10, 2023 09:55
Copy link
Collaborator

@xmo-odoo xmo-odoo left a comment

Choose a reason for hiding this comment

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

All the printing needs to be removed.

The treatment of return values is also inconsistent between maps and structs, it's not clear why e.g. recordFields in the last snippet is a []map[string][any] while in the second to last it's gets a map[string][struct{...}]. Or while the last one exists at all when no other snippet actually names that value.

Comment on lines 282 to 286
json, err := json.MarshalIndent(common, "", " ")
if err != nil {
log.Fatalln(err)
}
fmt.Println(string(json))
Copy link
Collaborator

Choose a reason for hiding this comment

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

The JSON output is indicative, no other client actually generates it. Remove output formatting in all snippets, it's an unnecessary waste of space.

Not to mention this doesn't format the structures to match the printed output.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Comment on lines 414 to 416
if err := models.Close(); err != nil {
log.Fatalln(err)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

So the models client gets closed, then we keep using it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

The requested changes have been made:
All json output formatting has been removed in all snippets;
The client is no longer closed;
No more output of results in all snippets.
@mrhdias
Copy link
Contributor Author

mrhdias commented Jul 10, 2023

The treatment of return values is not inconsistent. If we know what is returned we use a structure, if we don't know we use an interface{} which is the same as using the "any" expression. But if you wish, I can pass everything to interface{}.

@mrhdias mrhdias requested a review from xmo-odoo July 11, 2023 10:56
@xmo-odoo
Copy link
Collaborator

The treatment of return values is not inconsistent. If we know what is returned we use a structure, if we don't know we use an interface{} which is the same as using the "any" expression.

We know what e.g. read returns in the last snippet, it's literally printed below the code blocks. We also know e.g. what we get when we read the "name", "country_id", and "comment" fields of a partner.

mrhdias added 2 commits July 12, 2023 10:25
All go examples have been simplified. No more outputs in all go examples (including error output). Inconsistency between maps and structures has been eliminated.
A small typo in the authentication example has been fixed. Array replacement by map.
@mrhdias
Copy link
Contributor Author

mrhdias commented Jul 12, 2023

I think the examples are ok now. Check the latest revision. The Go examples closely follow the Java examples.

One more correction. The constants url, db, username and password were replaced by variables, so that they can be assigned with new values.
@AntoineVDV
Copy link
Collaborator

@robodoo delegate=xmo-odoo

@xmo-odoo
Copy link
Collaborator

I think the examples are ok now. Check the latest revision. The Go examples closely follow the Java examples.

The error handling was fine by me (most other languages would trigger exceptions on server-side error so the errors didn't need to he handled explicitely), I'm fine with approving as-is, or with adding the error checking stuff back in, as you prefer.

Now errors can be checked immediately before proceeding to the next steps.
@mrhdias
Copy link
Contributor Author

mrhdias commented Jul 13, 2023

I think the examples are ok now. Check the latest revision. The Go examples closely follow the Java examples.

The error handling was fine by me (most other languages would trigger exceptions on server-side error so the errors didn't need to he handled explicitely), I'm fine with approving as-is, or with adding the error checking stuff back in, as you prefer.

Thanks. I've added error checking again, so it's much clearer for developers to check for errors in each example. I followed this example from the go playground for errors.
https://go.dev/play/p/nPj4DTEuUnW

@xmo-odoo xmo-odoo changed the title Go Examples for External API v16 [ADD] rpc: Go examples Jul 14, 2023
Copy link
Collaborator

@xmo-odoo xmo-odoo left a comment

Choose a reason for hiding this comment

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

@xmo-odoo
Copy link
Collaborator

@robodoo squash

@robodoo
Copy link
Collaborator

robodoo commented Jul 14, 2023

Merge method set to squash.

@robodoo robodoo closed this in 0a65b08 Jul 14, 2023
@robodoo robodoo temporarily deployed to merge July 14, 2023 09:21 Inactive
robodoo pushed a commit that referenced this pull request Jul 14, 2023
Examples rely on `kolo/xmlrpc` as well as on the standard library package `log` to panic on error, matching snippets in other languages (which tend to raise exceptions).

As with other languages, only the RPC interaction is spelled out.

closes #5128

Forward-port-of: #5064
Signed-off-by: Xavier Morel (xmo) <[email protected]>
robodoo pushed a commit that referenced this pull request Jul 14, 2023
Examples rely on `kolo/xmlrpc` as well as on the standard library package `log` to panic on error, matching snippets in other languages (which tend to raise exceptions).

As with other languages, only the RPC interaction is spelled out.

closes #5126

Forward-port-of: #5064
Signed-off-by: Xavier Morel (xmo) <[email protected]>
robodoo pushed a commit that referenced this pull request Jul 14, 2023
Examples rely on `kolo/xmlrpc` as well as on the standard library package `log` to panic on error, matching snippets in other languages (which tend to raise exceptions).

As with other languages, only the RPC interaction is spelled out.

closes #5127

Forward-port-of: #5064
Signed-off-by: Xavier Morel (xmo) <[email protected]>
robodoo pushed a commit that referenced this pull request Jul 15, 2023
Examples rely on `kolo/xmlrpc` as well as on the standard library package `log` to panic on error, matching snippets in other languages (which tend to raise exceptions).

As with other languages, only the RPC interaction is spelled out.

closes #5129

Forward-port-of: #5064
Signed-off-by: Xavier Morel (xmo) <[email protected]>
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