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

Add flake8-error-message convention #129

Merged
merged 1 commit into from
May 17, 2023
Merged

Add flake8-error-message convention #129

merged 1 commit into from
May 17, 2023

Conversation

ThibFrgsGmz
Copy link
Contributor

Originating Project/Creator
Affected Component
Affected Architectures(s)
Related Issue(s)
Has Unit Tests (y/n)
Builds Without Errors (y/n)
Unit Tests Pass (y/n)
Documentation Included (y/n)

Change Description

This feature implements the flake8-err-msg which allows the formatting of nice error messages.

Rationale

The problem is that Python includes the line with the lift in the default traceback. This means that a user receives a message like this:

sub = "Some value"
raise RuntimeError(f"{sub!r} is incorrect")
Traceback (most recent call last):
  File "tmp.py", line 2, in <module>
    raise RuntimeError(f"{sub!r} is incorrect")
RuntimeError: 'Some value' is incorrect

If it is longer or more complex, the duplication can be confusing for a user not used to reading tracebacks.

On the other hand, if you always assign to something like msg, you get :

sub = "Some value"
msg = f"{sub!r} is incorrect"
raise RunetimeError(msg)
Traceback (most recent call last):
  File "tmp.py", line 3, in <module>
    raise RuntimeError(msg)
RuntimeError: 'Some value' is incorrect

There is now a more straightforward trace, less code and no double messages.

Ref - Ruff linter

Testing/Review Recommendations

Inspection is enough

Future Work

None

Copy link
Collaborator

@thomas-bc thomas-bc left a comment

Choose a reason for hiding this comment

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

Changes LGTM.

If this is a recommended standard I won't object to it at all, but just for the sake of raising a debate (which is something I like to do, in case you haven't realized by now 😄):
98% of the cases I agree that it declutters and this is a good thing, but there are edge cases where seeing the f-string can actually be a good thing, no?

e.g. say your args are badly formatted or something, and device is empty. With this change, you would get an error like this

raise ValueError(
                msg
            )
ValueError: Serial port ' ' not valid. Available ports:

Which really doesn't say much. Whereas if you keep the f-string in the raise statement, you get

raise ValueError(
                f"Serial port '{args['device']}' not valid. Available ports: {ports}"
)
ValueError: Serial port ' ' not valid. Available ports:

and this help to immediately identify where the issue is coming from. Thoughts? Do you know if flake8 addresses this?

@ThibFrgsGmz
Copy link
Contributor Author

Sorry for the delay. My computer has been down since Sunday morning (just after my PR as it happens...) and will probably be fixed this weekend when I can look more carefully at your comment!

@LeStarch
Copy link
Collaborator

We agreed that these changes are good, however, as a note: in F´  we expect to trap exceptions and present a clean error message to users without a confusing stack-trace. Thus the motivation here becomes one of appeasing a potentially good linter and less to beautify messages for users directly.

@thomas-bc thomas-bc merged commit a225fc7 into nasa:devel May 17, 2023
12 checks passed
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.

None yet

3 participants