Skip to content

Refine cutile-macro error#59

Merged
elibol merged 5 commits intoNVlabs:mainfrom
ur4t:cutile-macro-error
Apr 23, 2026
Merged

Refine cutile-macro error#59
elibol merged 5 commits intoNVlabs:mainfrom
ur4t:cutile-macro-error

Conversation

@ur4t
Copy link
Copy Markdown
Contributor

@ur4t ur4t commented Apr 1, 2026

No description provided.

@copy-pr-bot
Copy link
Copy Markdown

copy-pr-bot Bot commented Apr 1, 2026

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@ur4t ur4t force-pushed the cutile-macro-error branch from e2c2324 to 0d8471b Compare April 10, 2026 10:25
Copy link
Copy Markdown
Collaborator

@elibol elibol left a comment

Choose a reason for hiding this comment

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

Thanks for cleaning this up — the error API simplification and ok_or_else patterns look good.

One question: the change from .err("msg") (one step, returns Result) to .error("msg").into() (two steps) is more verbose at each call site. Was the motivation to make Error usable outside of Result contexts (e.g., error collection), or is there another reason for the split? Wondering if there's a way to keep the one-step ergonomics while still getting the benefits of the refactor.

Otherwise LGTM!

@ur4t
Copy link
Copy Markdown
Contributor Author

ur4t commented Apr 11, 2026

One question: the change from .err("msg") (one step, returns Result) to .error("msg").into() (two steps) is more verbose at each call site. Was the motivation to make Error usable outside of Result contexts (e.g., error collection), or is there another reason for the split? Wondering if there's a way to keep the one-step ergonomics while still getting the benefits of the refactor.

I started by replacing Err(syn_err(span, "msg") patterns and found an additional .error("msg") which produces Error is required for ok_or_else() and thought .err("msg") is a bit redundant.

Personally I perfer unified .error() and using .into() to produce Results, but I do not resist to bring .err("msg") back.

@ur4t ur4t force-pushed the cutile-macro-error branch from b652626 to 426e3d2 Compare April 13, 2026 03:39
@ur4t ur4t force-pushed the cutile-macro-error branch from 426e3d2 to 12ded13 Compare April 22, 2026 02:53
@elibol elibol merged commit d6f6da5 into NVlabs:main Apr 23, 2026
4 checks passed
@ur4t ur4t deleted the cutile-macro-error branch April 23, 2026 05:59
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.

2 participants