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

ExtensionError when validating dictionary includes error cause #1473

Merged
merged 4 commits into from
Oct 14, 2024

Conversation

austinyu12
Copy link

@austinyu12 austinyu12 commented Oct 9, 2024

#1019
Before, it only stated where the ExtensionError occurred.

Before
Screenshot 2024-10-08 at 11 12 29 PM

After
Screenshot 2024-10-08 at 11 12 37 PM

austinyu12 and others added 4 commits October 5, 2024 21:01
made the syntax error when validating a dictionary also provide the name of the term bank that contains the error
made the extension error when validating a dictionary provide the cause of error
@austinyu12 austinyu12 requested a review from a team as a code owner October 9, 2024 06:06
@MarvNC
Copy link
Member

MarvNC commented Oct 9, 2024

Can you edit a screenshot into the post comment

@austinyu12
Copy link
Author

I decided to throw the ExtensionError with all of the information contained within schema.errors. I attempted only returning the message property of schema.errors, but I decided against it. The code was already simple and will mainly be used for developers, so didn't seem like a problem if it contained a little bit of extra information, though if anyone has any idea on how that can be implemented, I would appreciate it. It also seems to be failing performance benchmarks.

@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 10, 2024
@Casheeew
Copy link
Member

It also seems to be failing performance benchmarks.

The benchmarking tool that we use was affected by the organizational move, so it is not working at the moment.

@djahandarie
Copy link
Collaborator

Hmm, ExtensionError doesn't support HTML internally right? It would probably be nice to format just the JSON with a monospace font (so that people know they can just skim over it, esp if they are not technical), but I guess that's not easy to accomplish with the current error type

@austinyu12
Copy link
Author

ExtensionError doesn't support HTML internally right?
ExtensionError seems to just extend Error. From what I can tell, I doesn't interact with the DOM directly.

@jamesmaa jamesmaa added this pull request to the merge queue Oct 14, 2024
Merged via the queue into yomidevs:master with commit 99d00d7 Oct 14, 2024
9 of 10 checks passed
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.

6 participants