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.

Sign up for GitHub

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

Jump to bottom

[IMP] make set of timeout-able hooks (and their timeouts) #1639

Merged
merged 1 commit into from
Sep 23, 2024
Merged

[IMP] make set of timeout-able hooks (and their timeouts) #1639

merged 1 commit into from
Sep 23, 2024

Conversation

xmo-odoo
Copy link
Contributor

Also unnest the handling of result via guard clauses, and generate messages as close as possible to use site, keeping the error construction itself where it currently is as the goal is specifically to point back to the definition site for the hook function.

@xmo-odoo
Copy link
Contributor Author

I also wanted to unify the error handling and inline onError by wrapping fn(...args) into a promise, but making it async makes the test suite very angry and I don't quite understand why, as the tests to seem to be handling async mounting / hooks.

@xmo-odoo
[IMP] make set of timeout-able hooks (and their timeouts) clearer by …
c460ef6 
…using a const map

Also unnest the handling of `result` via guard clauses, and generate
messages as close as possible to use site, keeping the error
construction itself where it currently is as the goal is specifically
to point back to the *definition* site for the hook function.
sdegueldre
sdegueldre approved these changes Sep 20, 2024
View reviewed changes
Copy link
Contributor

@sdegueldre sdegueldre left a comment

Choose a reason for hiding this comment

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

Definitely a good bit cleaner than it was. Is the purpose to make these timeouts configurable at some point?

@xmo-odoo
Copy link
Contributor Author

Definitely a good bit cleaner than it was. Is the purpose to make these timeouts configurable at some point?

Might be an option, initially I wanted to tune one of them to increase it before I remembered that this does not actually cause tests to fail, so that didn't really matter that much.

But I'd already started trying to make the code clearer as I was doing that so I figured I'd open a PR.

@sdegueldre
Copy link
Contributor

@ged-odoo I can't merge PRs on this repo anymore so this is probably for you

@ged-odoo
Copy link
Contributor

yeah, @sdegueldre your r+ privilege is suspended until you gave us some interesting stories...

@ged-odoo ged-odoo merged commit 2a22328 into odoo:master Sep 23, 2024
3 checks passed
@xmo-odoo xmo-odoo deleted the master-wrapError-impify-xmo branch September 24, 2024 05:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sdegueldre sdegueldre sdegueldre approved these changes

@ged-odoo ged-odoo ged-odoo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@xmo-odoo @sdegueldre @ged-odoo