search

vettloffah / odoo-await

Odoo API client featuring async await.
74 stars 29 forks source link

Using standard http and https ports? #19

Closed ajmas closed 2 years ago

ajmas commented 3 years ago

Currently the default port is 8069, though I wondering whether it would be worth using standard http (80) and https (443) ports, if the port is not provided in the configuration?

faryadmh commented 3 years ago

Hi, you can use blank ('') in port config

ajmas commented 3 years ago

Thanks. Is that documented somewhere?

At the same time would be good to have 0/undefined/null/‘’ all cause the protocol default port to be used, though admittedly that would break the 8069 assumption. I suppose a ‘useProtocolPort’ option could help, but maybe I’m not approaching this right?

In order to keep the value numerical, would it make sense to allow -1 to force the expected behaviour?

vettloffah commented 3 years ago

The reason the default port is set to 8069 is because that is the default port when running Odoo on local machine. E.g. if using this library on the same server that Odoo is running.

I guess it might make more sense to set it to the default http ports for connecting to a running instance online.

So yes, if you want to change the default port you can submit as PR and I can include in next major version change. I think this would be a breaking change for some users currently using the default port of 8069.

On Wed, Oct 20, 2021 at 9:15 AM Andre-John Mas @.***> wrote:

Thanks. Is that documented somewhere?

At the same time would be good to have 0/undefined/null/‘’ all cause the protocol default port to be used.

Would such a behaviour change be accepted as a PR?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/vettloffah/odoo-await/issues/19#issuecomment-947710313, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAYFFLALQ5WLE2XXEEJAS43UH3FGPANCNFSM5DETMEKQ .

ajmas commented 3 years ago

@vettloffah that I can understand, hence the suggestion of an alternative transitional approach to avoid breakages. Those being:

At the same time, since you did suggest limiting this change to a major release, I could make the a PR based on on the protocol default port. This is especially if we go with the philosophy that prod configurations should be the default behaviour?

vettloffah commented 3 years ago

I think the default being the http ports probably makes more sense since this library is usually going to be used to connect to an online running instance of odoo.

I can publish a new release soon with this update if you submit a PR, thanks!

On Wed, Oct 20, 2021 at 10:46 AM Andre-John Mas @.***> wrote:

@vettloffah https://github.com/vettloffah that I can understand, hence the suggestion of an alternative transitional approach to avoid breakages. Those being:

  • -1, to keep value numerical
  • an extra option such as useProtocolPort or useHttpPort (not sure about the names)

At the same time, since you did suggest limiting this change to a major release, I could make the a PR based on this. This is especially if we go with the philosophy that prod configurations should be the default behaviour?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/vettloffah/odoo-await/issues/19#issuecomment-947798631, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAYFFLD2MPPZSEZBPLOFCA3UH3P5FANCNFSM5DETMEKQ .

ajmas commented 3 years ago

I have made a PR, but for now have not updated the test due to not knowing how to run the tests on the master branch, as baseline.