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

📜 Improve error message structure and consistency #5616

Merged
merged 25 commits into from
Jun 27, 2024
Merged

Conversation

ArtV11
Copy link
Collaborator

@ArtV11 ArtV11 commented Jun 16, 2024

This pull request is part of a larger project in which we are redesigning our error messages according to these guidelines.

In this PR, I have worked on the redesigning topic (i) Structure. We aim to follow a consistent structure for all of our error messages. First sentence will inform our users of the issue, while the second sentence will offer a suggestion on how to resolve it. This approach helps users quickly understand where to look when something goes wrong and allows them to become accustomed to reading the error messages more efficiently due to their uniform structure. I added a suggestion on how to fix the problem for every error message while at it I also worked on (ii) Consistency. We aim to keep our error messages consistent by using the uniform wording and maintaining a similar sentence structure and length. Errors should follow the same format ensuring that the sentences start and end similarly. This consistency also aids in the readability and the structure of the error messages. because I was already creating new sentences while adding the suggestion and wanted to keep their wording consistent but the consistency aspect still needs some work for the future work.

Addresses #5012

Rtune11 and others added 5 commits May 21, 2024 15:15
FKEFFIEENFIENFFI
I tried to shorten them by using the requirements of Clarity and brevity requirements which are (aesthetic and minimalist design, recognition rather than recall). Meaning the errors should be shortened because students tend to not read the whole error so it should give out the critical info at the first part, that way a student can understand what it is going to say by not fully reading the whole sentence.

msgid "Has Blanks"
msgstr "Your code is incomplete. It contains blanks that you have to replace with code."
msgstr "Code is incomplete. Try filling in blanks with code."
Copy link
Member

Choose a reason for hiding this comment

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

Putting this on this one but it is a general remark, I don't believe this is better. It feels very abrasive and short, I miss the "human communication" that we had before, this feel very much like communication with a machine. Can we make it more conversational?


msgid "No Indentation"
msgstr "You used too few spaces in line {line_number}. You used {leading_spaces} spaces, which is not enough. Start every new block with {indent_size} spaces more than the line before."
msgstr "Insufficent {leading_spaces} spaces used on line {line_number}. Try increasing by {indent_size} for each new block."
Copy link
Member

Choose a reason for hiding this comment

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

Insufficient is a complex word! Maybe we should include target audience (10 and up) explicitly in the guidelines too and live by that?

Rtune11 and others added 3 commits June 18, 2024 18:17
Some suggestions on the first 7 error messages according to the given feedback
@Felienne Felienne changed the title Structure and working on consistency 📜 Structure and working on consistency Jun 25, 2024
@Felienne
Copy link
Member

I like the new structure, thanks! Will we do wording (like Insufficient) in this PR too? Otherwise it will block us from deploying.

Also (I am sure you know) tests are still failing, can you take a look?

Copy link
Member

@jpelay jpelay left a comment

Choose a reason for hiding this comment

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

Thanks for making them more consistent! I like the direction this is heading. I suggested some changes to some of the messages.

translations/en/LC_MESSAGES/messages.po Outdated Show resolved Hide resolved

msgid "Function Undefined"
msgstr "Function {name} used without being defined."
msgstr "We detected that a function {name} is being used without being defined. Can you define the function before it is used?"
Copy link
Member

Choose a reason for hiding this comment

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

Same thing with the backticks here, and replace a with the, because name will be replaced with an actual function name from the program.

translations/en/LC_MESSAGES/messages.po Show resolved Hide resolved
translations/en/LC_MESSAGES/messages.po Outdated Show resolved Hide resolved
translations/en/LC_MESSAGES/messages.po Outdated Show resolved Hide resolved

msgid "Missing Command"
msgstr "You missed a command on line {line_number}."
msgstr "We detected that the code seems to be missing a command on line {line_number}. Can you try looking at the exercise section to find which command to use?"
Copy link
Member

Choose a reason for hiding this comment

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

I find this suggestion interesting, there's not really something we call an exercise section, what did you mean by this?


msgid "Missing Inner Command"
msgstr "It seems you missed a command for the `{command}` statement on line {line_number}."
msgstr "We detected that the code seems to be missing a command for the `{command}` statement on line {line_number}. Can you try looking at the exercise section to find which command to use?"
Copy link
Member

Choose a reason for hiding this comment

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

Please replace statement with command everywhere you see it, or also remove the word altogether.

translations/en/LC_MESSAGES/messages.po Outdated Show resolved Hide resolved
translations/en/LC_MESSAGES/messages.po Outdated Show resolved Hide resolved

msgid "Wrong Number of Arguments"
msgstr "Wrong number of arguments for function {name}. Provided {used_number}, needs {defined_number}"
msgstr "We detected that function {name} has wrong number of arguments which is {used_number}. Can you try writing {defined_number} instead?"
Copy link
Member

Choose a reason for hiding this comment

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

Surround name with backticks

@ArtV11 ArtV11 marked this pull request as ready for review June 26, 2024 14:35
@Felienne Felienne changed the title 📜 Structure and working on consistency 📜 Improve error message structure and consistency Jun 27, 2024
Felienne and others added 3 commits June 27, 2024 14:11
Co-authored-by: Jesús Pelay <45865185+jpelay@users.noreply.github.com>
Co-authored-by: Jesús Pelay <45865185+jpelay@users.noreply.github.com>
Co-authored-by: Jesús Pelay <45865185+jpelay@users.noreply.github.com>
Felienne and others added 2 commits June 27, 2024 14:12
Co-authored-by: Jesús Pelay <45865185+jpelay@users.noreply.github.com>
Co-authored-by: Jesús Pelay <45865185+jpelay@users.noreply.github.com>
@Felienne
Copy link
Member

Hi @jpelay!

I am ok with all these changes, I am just not sure about the backtick doing what you think it does. If you can confirm it does, you may approve!

@Felienne
Copy link
Member

Hi @jpelay!

I am ok with all these changes, I am just not sure about the backtick doing what you think it does. If you can confirm it does, you may approve!

Never mind, all cleared up now!

Copy link
Contributor

mergify bot commented Jun 27, 2024

Thank you for contributing! Your pull request is now going on the merge train (choo choo! Do not click update from main anymore, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit 063d197 into main Jun 27, 2024
12 checks passed
Copy link
Contributor

mergify bot commented Jun 27, 2024

Thank you for contributing! Your pull request is now going on the merge train (choo choo! Do not click update from main anymore, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot deleted the Artun-test-branch branch June 27, 2024 14:39
@jpelay
Copy link
Member

jpelay commented Jun 27, 2024

I am ok with all these changes, I am just not sure about the backtick doing what you think it does. If you can confirm it does, you may approve!

Ohh sorry :c, I thought the backticks were necessary to show words as code in the error messages. What are they meant to do?

@Felienne
Copy link
Member

I am ok with all these changes, I am just not sure about the backtick doing what you think it does. If you can confirm it does, you may approve!

Ohh sorry :c, I thought the backticks were necessary to show words as code in the error messages. What are they meant to do?

I don't remember! Someone should read to code to figure it out :)

Ok, I dove in a bit. I think you are right, partly, it does work like that, but there is also back-end magic involved as I thought, see here:

def _highlight(input_):

So it is not like just simple `` in markdown, which is what I remembered.

@jpelay
Copy link
Member

jpelay commented Jun 27, 2024

Ok, I dove in a bit. I think you are right, partly, it does work like that, but there is also back-end magic involved as I thought, see here:

def _highlight(input_):

So it is not like just simple `` in markdown, which is what I remembered.

Ahhhhh, I see! Thanks for clarifying!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants