no-chris / chord-symbol

The definitive chord symbol parser and renderer for Javascript/NodeJS.
https://chord-symbol.netlify.app
MIT License
175 stars 8 forks source link

Explain why the chord parsing fails #337

Closed no-chris closed 3 years ago

no-chris commented 3 years ago

Would be great to return an explanation when a chord fails to parse

@infojunkie since this is a suggestion of yours what API would you expect? return an error object, an error string, throw an exception, log an error, ...?

infojunkie commented 3 years ago

I prefer exceptions to error returns, but that's a personal preference. In any case, it would be important to include:

A log is useful, because it can also contain non-errors, such as info, warnings, etc. but that's a different feature :sweat_smile:

no-chris commented 3 years ago

I find the exception a bit violent for this use case. After looking at some popular libraries I find the "error property" approach fit for the current library.

In short, I would remove the null return and always return an object with at least an isValid property and possibly an error property. They would be filled as specified below:

Case Example Expected output
The string cannot be confused with a Chord "Zorro" chord.isValid: false / chord.error: undefined
The string could be confused with a Chord, but the parsing rules fail "Amish", "A6b6" chord.isValid: false / chord.error: errorMsg
The string is a chord "Ami" chord.isValid: true / chord.error: undefined

I'm not sure how big is the need to distinguish between case 1 and 2. It serves the use case of needing to detect if a given string is a chord. In that case you might not want to feedback an error if it is clear from the start that there is no possible confusion.

Wdyt of the approach?

infojunkie commented 3 years ago

Sounds like a great approach. I agree, exceptions are more suited to in-module error handling, but not across module boundaries.

I'm not sure how big is the need to distinguish between case 1 and 2.

I would suggest also returning a specific error message for an input that cannot be confused with a chord. Simplistically, in case 1, the chord root is invalid, whereas in case 2, the chord modifiers are invalid. Of course the more specific the error messages the better!

no-chris commented 3 years ago

Thanks for the feedback 🙌

I would suggest also returning a specific error message for an input that cannot be confused with a chord.

I'm thinking that an error object with error codes might be better then. It will simplify the API while serving the same use cases, and also allow to give a explicit message for case 1. Something like:

Case Example Expected output
The string cannot be confused with a Chord "Zorro" chord.error: { code: 'notAChord'; msg: '{input} does not seem to be a chord' }
The string could be confused with a Chord, but the parsing rules fail "Amish", "A6b6" chord.error: { code: 'parsingError'; msg: 'xxx' }
The string is a chord "Ami" chord.error: undefined }
no-chris commented 3 years ago

closed with #360