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

syntax error message made more verbose #1462

Merged
merged 2 commits into from
Oct 6, 2024
Merged

Conversation

austinyu12
Copy link

when validating a dictionary as it is being imported, if a syntax error is encountered, a message would describe the syntax error that has occurred. To provide clarity, the syntax error message will now also describe the file in which this error occurred.

made the syntax error when validating a dictionary also provide the name of the term bank that contains the error
@austinyu12 austinyu12 requested a review from a team as a code owner October 6, 2024 04:10
@MarvNC
Copy link
Member

MarvNC commented Oct 6, 2024

Can you add screenshots of before and after?

@austinyu12
Copy link
Author

Before
Screenshot 2024-10-05 at 9 16 09 PM
After
Screenshot 2024-10-05 at 9 19 30 PM

catch (error) {
if (error instanceof Error) {
let newError = new Error(error.message + `. Dictionary has invalid data in '${file.filename}'`);
console.error(newError);
Copy link
Member

Choose a reason for hiding this comment

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

You should use log.error instead. log is a wrapper around console.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you. Can you elaborate on what makes log.error more appropriate?

Copy link
Member

Choose a reason for hiding this comment

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

You can check out https://github.com/themoeway/yomitan/blob/master/ext/js/core/log.js#L74. It basically formats the log with info for the extension.

catch (error) {
if (error instanceof Error) {
let newError = new Error(error.message + `. Dictionary has invalid data in '${file.filename}'`);
console.error(newError);
Copy link
Member

Choose a reason for hiding this comment

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

In addition, I don't think it's useful logging here and still letting the error propagate. If I decided to log the errors upstream, it would cause the error to be logged twice. You can remove the console.error entirely.

Copy link
Author

Choose a reason for hiding this comment

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

Ok. I'll do away with it entirely.

@austinyu12
Copy link
Author

Made changes

@Kuuuube Kuuuube added this pull request to the merge queue Oct 6, 2024
Merged via the queue into yomidevs:master with commit 73793a4 Oct 6, 2024
11 checks passed
@Kuuuube Kuuuube added kind/enhancement The issue or PR is a new feature or request area/ui-ux The issue or PR is related to UI/UX/Design labels Oct 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ui-ux The issue or PR is related to UI/UX/Design kind/enhancement The issue or PR is a new feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants