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

Switch to hard-coded error messages #2

Merged
merged 3 commits into from
Oct 19, 2021

Conversation

adrianeboyd
Copy link
Contributor

No description provided.

@adrianeboyd adrianeboyd requested a review from svlandeg October 18, 2021 08:42
Copy link
Member

@svlandeg svlandeg left a comment

Choose a reason for hiding this comment

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

I see why we'd want to have these locally, but how do we plan on numbering them in the future, making sure there's no duplicate usage of the same number?

@adrianeboyd
Copy link
Contributor Author

Good point, I guess the error codes could be customized per package, but then we have to customize add_codes. Hmm...

@adrianeboyd
Copy link
Contributor Author

Or we have hard-coded error messages without codes here that are copied at the point when this was transferred?

@adrianeboyd
Copy link
Contributor Author

Or maybe we can just customize ENNN?

@adrianeboyd
Copy link
Contributor Author

Not the most attractive error message I've ever seen, but eh?

@adrianeboyd
Copy link
Contributor Author

I don't think the other packages use error codes, so maybe hard-coding is easier.

Ah, this is also in spacy-legacy. But if this becomes a more independent package, we need something else than maintaining the errors in spacy, which is going to be a headache.

@svlandeg
Copy link
Member

Hardcoding would actually be fine by me. Like you said we need to be able to update this package independently, it's going to be a hassle otherwise.

@adrianeboyd adrianeboyd changed the title Use local error messages, fix error references Switch to hard-coded error messages Oct 19, 2021
@adrianeboyd adrianeboyd merged commit 16f536c into explosion:main Oct 19, 2021
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