Kino.Input error messages #327
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rhcarvalho
force-pushed
the
date-range
branch
from
4f3f4c0
to
b8a02c3
Compare
|
@jonatanklosko I reviewed the existing error messages and based on your feedback updated the messages for all related functions:
Since the "expected ..." format is used in many places, I kept that + your suggestions. I dropped the tests-related commit so that this PR can be focused on a single thing. |
rhcarvalho
changed the title
jonatanklosko
approved these changes
Sep 11, 2023
rhcarvalho
force-pushed
the
date-range
branch
from
b8a02c3
to
74d3d89
Compare
For clarity, make the messages more consistent and always include the argument value that caused the error, making debugging easier. Replace "less than" and "greater than" language for dates with "before" and "after". The re-wording also pays closer attention to the three possible cases: before, equal, or after.
rhcarvalho
force-pushed
the
date-range
branch
from
74d3d89
to
93f27b5
Compare
Done! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Hi there!
@jonatanklosko was faster than I in addressing #325 with #326 -- thanks for the quick fix!
Since I had some changes laying around already, I decided to post them here for feedback/discussion, I wouldn't mind if they are eventually not accepted.
TestsI noticedKino.Input.datewas lacking tests, and was happy to see @jonatanklosko added one yesterday!I had a plan to cover more of the possible cases, and so tried some form of table-testing to reduce verbosity. I wonder how others see this type of test code in Elixir, given it also adds some cognitive complexity (unquote,Macro.escape, ...).The constraints in this case are simple enough that property-based testing would also be a fun exercise :-)Update: removed tests to keep the PR more focused.
Error messages
Two out of three messages were missing part of what caused the error, making debugging unnecessarily harder.
I tried to make the messages more obvious and easier to interpret by a human when their Livebook cell is spitting out an error on them.
Something like
expected :min to be less than :max, got: #{inspect(min)} and #{inspect(max)}is not exactly correct, as:mincan also be equal to:max. If I were to continue the existing mathematical pattern, I'd write:But, then, I thought why not simply state the problem directly:
:min is greater than :max. And, when we talk about dates and time, I think the words "before" and "after" might be a better fit (might be a bias from my Go experience, where thetimepackage uses the words "before" and "after").I would love to hear what are the Elixir-way ideas here, specially if there is a well establish preference.
If my changes are welcome, I could also update the other messages (time, etc) to match.