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.

Sign up for GitHub

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

Jump to bottom

Standardize error messages in TLuaInterpreter.cpp #2091

Closed
Kebap opened this issue Oct 28, 2018 · 7 comments
Closed

Standardize error messages in TLuaInterpreter.cpp #2091

Kebap opened this issue Oct 28, 2018 · 7 comments
Assignees
@Kebap
Labels
help wanted wishlist
Milestone
5.0 beginner-frie...

Comments

@Kebap
Copy link
Contributor

Kebap commented Oct 28, 2018 •
edited

Brief summary of issue / Description of requested feature:

Some (but not all) functions give basic information on failure, for example: which argument is not as expected (if there are multiple arguments) and what would have been expected instead. This should be expanded as best practise for all functions.

Reasons for adding feature:

  1. Current best practise seems to be something like "getFontSize: bad argument Profile keyboard navigation #1 type (window name as string expected, got %s!)".
  2. Worst practise example is like "createLabel: wrong argument type", where you are expected to give 6 different arguments, and will never learn which one failed or why.
  3. Sentence structure of above "best practise" example unfortunately does not seem gramatically perfect or easy to understand for non-English speakers.

Expected result of feature

A good practise seems to consist of following elemets:

  • Obviously easy to understand English sentence structure with all needed elements included (e.g. "expected X, got Y")
  • number of failed argument (e.g. "Profile keyboard navigation #1")
  • meaning of said argument (e.g. "window name")
  • expected type of said argument (e.g. "string")
  • received type of said argument (added dynamically, e.g. "%s")
  • optionally, an indication whether or not said argument was optional (not included in example above)

Ideas for how to solve / implement:

  • English sentence structure should be improved for added understandability, like so: "(Optional) Argument Profile keyboard navigation #1 (window name) is of wrong type! Expected string, got %s"
  • To avoid uncontrolled growth of different error message styles again, creation of said message should be centralised in a helping function, which only receives details as listed above, and returns the full string. Therefore, future adjustments to sentence structure can be delivered once and for all easily.
@Kebap Kebap added this to the 5.0 beginner-friendly milestone Oct 28, 2018
@Kebap
Copy link
Contributor Author

Kebap commented Oct 28, 2018

Other error messages do not talk about wrong type, but wrong value.

Example for best practise yet: "setBackgroundColor: bad argument #%d value (red value needs to be between 0-255, got %d)"

Suggestion along lines of above: "(Optional) Argument #%d (red value) has unexpected value! Expected value needs to be between 0-255, got %d"

@SlySven
Copy link
Member

SlySven commented Oct 31, 2018

Actually the wrong value cases (colloquially, or at least by me, called run-time errors because the supplied arguments are of the right type but not the right values to produce valid results) are already starting to be redone and moving away from the example quoted in the last post. They:

  • do not mention any longer the function where the problem has occurred (setBackgroundColor: in the example above, because, IIRC according to Vadim the user knows what the function is so there is no need to repeat it).
  • should not begin with a capital letter or have terminal punctuation because they are capable of being handled by the calling function (they do not call lua_error(L) to abort execution) and may wrapped in additional text for presentation purposes.
  • should explain in friendly terms what is wrong, what the presented value was (if applicable) and what the correct range of values would be (again, if applicable, but probably only really relevant for numeric arguments).

With regard to the initial issue report I would note that the use of the phrase as XXXX expected, got YYYY! indicates a required argument in those messages about the wrong type - I've just counted something of the order of 180 examples of that in the TLuaInterpreter.cpp file... and for optional arguments the text will be as XXXX is optional, got YYYY! and I've spotted 20 of those right now. 😀

@Kebap
Copy link
Contributor Author

Kebap commented Dec 1, 2020 •
edited

Here follows an inventory of TLuaInterpreter's public functions
(there are other files establishing public functions as well, but this seems to be the largest flock)
Source: TLuaInterpreter::initLuaGlobals

Position in following groups indicate whether error messages are (somewhat) standardized:

✅ These functions have been fixed now

(to be filled from below)

⚡ Error messages to be standardized indeed

Source-code needs to be edited, messages need to be improved. Afterwards, move to above "fixed" section and/or set check-mark

✅ These functions were already standardizing messages alright
✅ Internal functions - unknown to users
✅ Functions without arguments - fine already! 😄

@Kebap Kebap mentioned this issue Dec 1, 2020
Merged
@Kebap
Copy link
Contributor Author

Kebap commented Dec 7, 2020 •
edited

Verification is not yet complete, but already found 65 functions needing work. ⚡
My last message above is continuously updated with the latest status for each.

To avoid uncontrolled growth of different error message styles again, creation of said message should be centralised in a helping function, which only receives details as listed above, and returns the full string. Therefore, future adjustments to sentence structure can be delivered once and for all easily.

Not sure if this refactorization can be done prior or is out of scope here.
Danger if not done lies in sandardized error messages today, tomorrow having wild growth again.

@Kebap
Copy link
Contributor Author

Kebap commented Dec 18, 2020

Verification done. After looking at these 400 functions published from TLuaInterpreter for a few more hours, we have a total of 80 functions to fix. You can find the whole list behind the spoiler tags in my comment above. Any volunteers?
grafik

There were also a few slips and careless mistakes which could have been avoided with my suggested refactoring in the OP.
E.g. function X reporting itself as function Y instead (copy/paste error probably, but actually unnecessary duplication of data)

@Kebap Kebap mentioned this issue Dec 18, 2020
Merged
@Kebap Kebap mentioned this issue Jan 7, 2021
Merged
@Kebap Kebap self-assigned this Jan 7, 2021
@Kebap Kebap mentioned this issue Jan 8, 2021
Merged
@Kebap Kebap mentioned this issue Jan 15, 2021
Merged
@Kebap
Copy link
Contributor Author

Kebap commented Jan 25, 2021

While there is still ongoing work internally to streamline and standardize text creation, this issue is mostly done.

The players only see the "wrong argument type" style error message in setPopup() and no other function anymore.

As explained in #4599 (comment) there are differences in code and wiki documentation still

@Kebap
Copy link
Contributor Author

Kebap commented Feb 2, 2021

I will mark this as done. The only function remaining (setPopup) is discussed in a seperate issue #4641 still.

Meanwhile we established the requested helper functions and gave meaningful type errors and value errors to all c++ functions.

Checked Lua functions, too. Not all of them check all their arguments already, but if they do, they give informative error messages.

@Kebap Kebap closed this as completed Feb 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Kebap Kebap

Labels
help wanted wishlist
Projects
None yet
Milestone
5.0 beginner-friendly
Development

No branches or pull requests
2 participants
@Kebap @SlySven