Skip to content

Conversation

@dejurin
Copy link
Contributor

@dejurin dejurin commented Feb 18, 2025

The "return" already implies an error.

Error strings should not be capitalized (unless beginning with proper nouns or acronyms) or end with punctuation, since they are usually printed following other context.

Remove "Error", 
The "return" already implies an error.
Error strings should not be capitalized (unless beginning with proper nouns or acronyms) or end with punctuation, since they are usually printed following other context.
Remove "Error", 
The "return" already implies an error.
Error strings should not be capitalized (unless beginning with proper nouns or acronyms) or end with punctuation, since they are usually printed following other context.
@philippta
Copy link
Owner

This error is visible to the user in its bare form, so I would rather keep the "Error" prefix.
Otherwise the user would just see this message:

$ flyscrape dev script.js
parsing config flags: <actual error message here>

@dejurin
Copy link
Contributor Author

dejurin commented Feb 18, 2025

How about this?

return fmt.Errorf("config: error parsing flags: %w", err)

or

return fmt.Errorf("error parsing config flags: %w", err)

@philippta
Copy link
Owner

We could make a small improvement by aligning it with the naming scheme from here, as those would also be user visible:
https://github.com/philippta/flyscrape/blob/master/flyscrape.go

@dejurin
Copy link
Contributor Author

dejurin commented Feb 18, 2025

as example of prefix:

// config: - configuration errors
// script: - script problems
// module: - errors in modules
// http: - network errors
// filesystem: - problems with the file system

@philippta
Copy link
Owner

While I like prefixes in general for logging or debugging (like in long running server applications), I wouldn’t put any of these kinds of prefixes on user visible errors. I don’t see many cli tools having prefixes in their error messages.

User visible errors should be as human readable and understandable as possible, like a regular English sentence.

@dejurin
Copy link
Contributor Author

dejurin commented Feb 18, 2025

I got it. So, then I suggest, fix only the small letter at the beginning.

@philippta
Copy link
Owner

LGTM

@philippta philippta merged commit f1084f3 into philippta:master Feb 18, 2025
1 check 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.

2 participants