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

Editoast error management #237

Merged
merged 1 commit into from
Feb 3, 2025
Merged

Editoast error management #237

merged 1 commit into from
Feb 3, 2025

Conversation

leovalais
Copy link
Contributor

Settling on this topic is necessary in order to go forward with the split in crates.

@leovalais leovalais marked this pull request as ready for review September 18, 2024 09:27
woshilapin
woshilapin reviewed Sep 19, 2024
View reviewed changes
Copy link
Contributor

@woshilapin woshilapin left a comment

Choose a reason for hiding this comment

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

Here is a first round, hope you like it. I'm really worried by the complexity of this macro and if there would be some other solution.

content/docs/reference/design-docs/editoast-errors/_index.en.md Outdated Show resolved Hide resolved
content/docs/reference/design-docs/editoast-errors/_index.en.md Outdated Show resolved Hide resolved
content/docs/reference/design-docs/editoast-errors/_index.en.md Outdated Show resolved Hide resolved
content/docs/reference/design-docs/editoast-errors/_index.en.md Outdated Show resolved Hide resolved
content/docs/reference/design-docs/editoast-errors/_index.en.md Outdated Show resolved Hide resolved
content/docs/reference/design-docs/editoast-errors/_index.en.md Outdated Show resolved Hide resolved
content/docs/reference/design-docs/editoast-errors/_index.en.md Outdated Show resolved Hide resolved
content/docs/reference/design-docs/editoast-errors/_index.en.md Outdated Show resolved Hide resolved
content/docs/reference/design-docs/editoast-errors/_index.en.md Outdated Show resolved Hide resolved
content/docs/reference/design-docs/editoast-errors/_index.en.md Show resolved Hide resolved
@leovalais
Copy link
Contributor Author

Thanks for all the corrections! I'll get back to this document when I have some time, hopefully in the next iteration :)

@leovalais
Copy link
Contributor Author

I've added a bit of content, especially an implementation plan and details about how to forward Core errors.

@leovalais
Copy link
Contributor Author

Thanks for the corrections!

woshilapin
woshilapin reviewed Dec 2, 2024
View reviewed changes
Copy link
Contributor

@woshilapin woshilapin left a comment

Choose a reason for hiding this comment

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

Nice introducing some new sections about "Issues of the old system" or "Implementation plan".

content/docs/reference/design-docs/editoast-errors/_index.en.md Outdated Show resolved Hide resolved
content/docs/reference/design-docs/editoast-errors/_index.en.md Outdated Show resolved Hide resolved
@leovalais
Copy link
Contributor Author

This looks mergeable as it is to me.

@Khoyo: we discussed about Core errors with @flomonster yesterday and settled on wrapping the errors, but not parsing them. Lmk if it's ok.

@Khoyo @woshilapin: do I drop the "Rejected Ideas" section or not? (Personally I don't care.)

A final round of review would be much appreciated 🙏

woshilapin
woshilapin reviewed Jan 7, 2025
View reviewed changes
Copy link
Contributor

@woshilapin woshilapin left a comment

Choose a reason for hiding this comment

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

Sorry, made a few more comments, I believe most of them are not fundamental so should not move things too much.

And for your questions, I'd prefer to keep "Rejected ideas", it's a good thing to keep these around so we avoid to reinvent things we might have explore in the past.

content/docs/reference/design-docs/editoast-errors/_index.en.md Outdated Show resolved Hide resolved
content/docs/reference/design-docs/editoast-errors/_index.en.md Outdated Show resolved Hide resolved
content/docs/reference/design-docs/editoast-errors/_index.en.md Show resolved Hide resolved
content/docs/reference/design-docs/editoast-errors/_index.en.md Outdated Show resolved Hide resolved
content/docs/reference/design-docs/editoast-errors/_index.en.md Outdated Show resolved Hide resolved
content/docs/reference/design-docs/editoast-errors/_index.en.md Outdated Show resolved Hide resolved
content/docs/reference/design-docs/editoast-errors/_index.en.md Outdated Show resolved Hide resolved
content/docs/reference/design-docs/editoast-errors/_index.en.md Outdated Show resolved Hide resolved
content/docs/reference/design-docs/editoast-errors/_index.en.md Outdated Show resolved Hide resolved
content/docs/reference/design-docs/editoast-errors/_index.en.md Outdated Show resolved Hide resolved
@leovalais leovalais merged commit 0faad55 into master Feb 3, 2025
4 checks passed
@leovalais leovalais deleted the editoast-errors branch February 3, 2025 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SergeCroise SergeCroise SergeCroise left review comments

@Khoyo Khoyo Khoyo left review comments

@woshilapin woshilapin woshilapin approved these changes

@flomonster flomonster Awaiting requested review from flomonster

@Tguisnet Tguisnet Awaiting requested review from Tguisnet

Assignees

@leovalais leovalais

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@leovalais @Khoyo @woshilapin @SergeCroise