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

Simplify and improve the error argument of Rephraser #54

Merged
merged 4 commits into from
Jun 23, 2021

Conversation

janluke
Copy link
Owner

@janluke janluke commented Jun 23, 2021

The error argument is of type Union[None, str, ErrorRephraser].

ErrorRephraser was redefined (breaking change) from:

(ctx: Context, constraint: Constraint, params: Sequence[params]) -> str

to:

(err: ConstraintViolated) -> str

This was possible without making it less powerful by enriching ConstraintViolated, which now takes all three arguments that ErrorRephraser took separately: ctx, constraint and params. (breaking change)

This also means that an ErrorRephraser can now easily reuse the original error, which is useful if all you want is to add extra info before or after the original error! Example:

mutually_exclusive.rephrased(
    error=lambda err: f"{err}.\nUse --renderer, the other two options are deprecated."
)(
    option(
        "--renderer",
        type=click.Choice(["cairo", "opengl", "webgl"], case_sensitive=False),
        help="Select a renderer for your Scene.",
    ),
    option(
        "--use_opengl_renderer", is_flag=True, hidden=True,
        help="(Deprecated) Use --renderer=opengl.",
    ),
    option(
        "--use_cairo_renderer", is_flag=True, hidden=True,
        help="(Deprecated) Use --renderer=cairo.",
    ),
)

The original message is also available as format string field {error}. So the following is equivalent to the above code:

mutually_exclusive.rephrased(
    error="{error}.\nUse --renderer, the other two options are deprecated."
)(...)

- Make ctx required
- Add argument constraint and params

This change aims at making ConstraintViolated a
complete source of information about the error,
so that ErrorRephraser can be simplified taking
just a single argument (the error itself).
@codecov-commenter
Copy link

codecov-commenter commented Jun 23, 2021

Codecov Report

Merging #54 (49dddac) into master (1599195) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #54   +/-   ##
=======================================
  Coverage   96.28%   96.29%           
=======================================
  Files          20       20           
  Lines        1291     1294    +3     
=======================================
+ Hits         1243     1246    +3     
  Misses         48       48           
Impacted Files Coverage Δ
cloup/constraints/__init__.py 100.00% <100.00%> (ø)
cloup/constraints/_conditional.py 97.77% <100.00%> (ø)
cloup/constraints/_core.py 96.09% <100.00%> (ø)
cloup/constraints/exceptions.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1599195...49dddac. Read the comment docs.

@janluke janluke force-pushed the feat/improve-error-rephraser branch from f1ffc6f to 49dddac Compare June 23, 2021 00:35
@janluke janluke changed the title Simplify and improve ErrorRephraser Simplify and improve the error argument of Rephraser Jun 23, 2021
@janluke janluke merged commit e6dd178 into master Jun 23, 2021
@janluke janluke deleted the feat/improve-error-rephraser branch June 23, 2021 00:53
@janluke janluke added this to the v0.9.0 milestone Jun 23, 2021
@janluke janluke added breaking change This breaks backward compatibility constraints enhancement New feature or request labels Jun 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change This breaks backward compatibility enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants