Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Use
PublishError
along publishing #190Use
PublishError
along publishing #190Changes from 2 commits
1ddddab
ee0745b
47cd409
4502143
6c891cf
fea6de3
84be3d4
ece72b3
65249cc
bde4ab3
0363a57
bb44614
452f73e
ce5a314
6de4af0
33c2bb3
15dbe43
a03e49b
8bb8bf7
107012c
d92689e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This provides way too little detail to be any meaningful error to any artist or alike. What destination where, for which key - and what did it get instead? We're really (I feel) going the wrong way if this can't just be a
TypeError
but requires aPublishError
since we suddenly are introducing a super generic error much less clear? (I mean, still better than the assert, don't get me wrong)@Illicit thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should every error (that can be raised along the publishing) be a
PublishError
?I've copula of scattered thoughts.
PublishError
orPublishValidationError
will cause the publisher UI to show the message (It's not your fault etc).PublishError
andPublishValidationError
. These errors can be fixed by the user.PublishError
can help developers know about the problem from a screenshot. (Note: I didn't use the title in thePublishError
so the publish plugin's label will be used.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup - this was a design decision at some point. I'm still not 100% confident that it's the right one, but it did force us into writing better
PublishValidationError
. If now we're just going to make EVERYTHINGPublishError
without providing better messages I think we're in the same problematic realm and we could basically revert to allow any error be nicely reported - we'll just still need to make sure that the majority of errors we raise are very clear what do to about them.I can't think of any now - but that does sound right. However, the error must be very clear, describe what is wrong and how to go about fixing it, etc.
Some assertions in the codebase were just there - to assert for a potential edge case where it might happen... not necessarily that they would happen. That's why some might not necessarily be bad to remain asserts (or other regular errors). They were just there because we just really wanted to make sure if the code continued that something behaved a certain way.