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

Use a floating-point number for the timeout property in HTTPRequest #60402

Merged
merged 1 commit into from
May 20, 2022

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented Apr 20, 2022

This allows for greater precision when specifying a timeout in HTTPRequest in 3.x, similar to what is already possible in master.

This also improves the documentation (which could also be added to master).

See godotengine/godot-proposals#4419.

@Calinou Calinou requested review from a team as code owners April 20, 2022 22:18
@Calinou Calinou force-pushed the httprequest-timeout-float branch from be19eca to 551a0a2 Compare April 20, 2022 22:18
@Calinou Calinou added this to the 3.5 milestone Apr 20, 2022
@akien-mga akien-mga requested a review from a team April 26, 2022 09:56
Copy link
Collaborator

@Faless Faless left a comment

Choose a reason for hiding this comment

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

See my suggestions to avoid CMP_EPSILON for readability.

scene/main/http_request.cpp Outdated Show resolved Hide resolved
scene/main/http_request.cpp Outdated Show resolved Hide resolved
scene/main/http_request.cpp Outdated Show resolved Hide resolved
This allows for greater precision when specifying a timeout in
HTTPRequest in `3.x`, similar to what is already possible in `master`.
@Calinou Calinou force-pushed the httprequest-timeout-float branch from 551a0a2 to 069c955 Compare May 11, 2022 06:41
@akien-mga akien-mga requested a review from Faless May 11, 2022 07:20
@Faless
Copy link
Collaborator

Faless commented May 11, 2022

A was about to merge this, but isn't this a script-breaking change?

func some_func():
  var timeout : int = $HTTPRequest.timeout

I guess it's a rare instance, but it's worth mentioning maybe?

@akien-mga
Copy link
Member

A was about to merge this, but isn't this a script-breaking change?

func some_func():
  var timeout : int = $HTTPRequest.timeout

I guess it's a rare instance, but it's worth mentioning maybe?

In GDScript, it seems to be happy to do implicit conversion:

	var timeout : int = 1.8
	print(timeout)   # 1

But that would likely be more breaking for C# and GDNative indeed.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

After discussion, we found that the slight compat breakage is ok for 3.5 and it will be pointed out in the # Changed section of the changelog, together with a few other theoretically API breaking changes done in 3.5 (like the new Navigation API).

@akien-mga akien-mga merged commit 67208ae into godotengine:3.x May 20, 2022
@akien-mga
Copy link
Member

Thanks!

@Calinou Calinou deleted the httprequest-timeout-float branch May 20, 2022 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants