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

Clean up LOG/PRINT functions. Fixes #5004. #5006

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jordanbrown0
Copy link
Contributor

Move the error-log output next to the console output. (This means that error-log entries are suppressed after five identical ones. Is that good?)

Move throwing on hardwarning/error out of the PRINT() functions and into its own function, and have LOG() call that function. (Fixes #5004.)

Along the way, clean up a little: nobody outside of printutils.cc should be calling PRINT_NOCACHE(), so scope it static. Remove PRINTB_NOCACHE since it's unused and its one pseudo-use in a comment looks like the intent is the same as PRINTD().

@jordanbrown0
Copy link
Contributor Author

Sigh. A fair number of tests depend on Error not being fatal. I could have Error be similar to Warning, fatal only if hardwarnings. Or fix those tests. Or maybe add a separate harderrors. Thoughts? Shelved for the moment pending comments on this and on the "suppress >5 duplicates" question above.

@thehans
Copy link
Member

thehans commented Mar 5, 2024

Do we have a clear differentiation of what types of conditions should trigger an ERROR vs a WARNING?

Looking at the failing tests report, it appears that all the errors that terminate the tests are related to converting invalid data to points:

ERROR: Unable to convert points[4] = [0, 0, -inf] to a vec3 of numbers in file builtin-invalid-range-test.scad, line 16
...
ERROR: Unable to convert points[3] = [30] to a vec2 of numbers in file errors-warnings.scad, line 3
...
ERROR: Unable to convert points = undef to a vector of coordinates in file allmodules.scad, line 25
...
ERROR: Unable to convert points = undef to a vector of coordinates in file text-search-test.scad, line 15
...
ERROR: Unable to convert points = undef to a vector of coordinates in file polygon-tests.scad, line 1

Would it make any more sense to have those as WARNINGS? What should ideally qualify as an ERROR?

@jordanbrown0
Copy link
Contributor Author

I don't have any real doubt that some cleanup is appropriate on what's a warning versus an error, but that seems orthogonal to the issues here around how each of them is handled.

I'd say that an error is when the result cannot be correct, and a warning is where it might or might not be right but is in some way suspicious. Deprecations (e.g. behavior is slated for removal, behavior is known to be confusing and has been replaced) are the obvious candidates for warnings.

No matter the distinction, there will be those who will say "why did you stop my run, it worked fine before this, get off my lawn", even though there's evidence that something is not working the way they expect. That's why we probably need an option to not stop on errors that aren't totally fatal. (Parse errors might be totally fatal.)

@kintel
Copy link
Member

kintel commented Mar 10, 2024

As discussed offline, should we just get the NOCACHE fix merged before delving into the details of errors and warnings?

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.

Doesn't stop execution on errors
3 participants