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

Improved logging, fix unicode error, include_dir buildrun #253

Merged
merged 3 commits into from
Feb 12, 2024

Conversation

incaseoftrouble
Copy link
Contributor

@incaseoftrouble incaseoftrouble commented Feb 12, 2024

This replaces #209 (I migrated the repository) and fixes #201

@incaseoftrouble incaseoftrouble changed the title WIP Improved logging, fix unicode error, include_dir buildrun Improved logging, fix unicode error, include_dir buildrun Feb 12, 2024
@incaseoftrouble
Copy link
Contributor Author

incaseoftrouble commented Feb 12, 2024

Okay, I'm quite happy with this now.

Changes:

  • Use (named) loggers where applicable, in particular each problem aspect now has their own name
  • Building can now include files (e.g. if you have a common build script to build a java-based tool, you don't need to copy it around)
  • Fixed a problem with _JUNK_CASES: Bytes larger than 127 are not valid UTF-8, so validators could fail with UTF8DecodeError (see UnicodeDecodeError problems due to non-parseable _JUNK_CASES #201 )
  • Add a bit more logging here and there

Copy link
Contributor

@pehrsoderman pehrsoderman left a comment

Choose a reason for hiding this comment

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

Minor comment regarding pep8.

@pehrsoderman pehrsoderman merged commit b6866d1 into Kattis:develop Feb 12, 2024
1 check passed
Tagl added a commit to Tagl/problemtools that referenced this pull request Feb 15, 2024
@simonlindholm
Copy link
Member

Use (named) loggers where applicable, in particular each problem aspect now has their own name

I've found that this makes the output look rather uglier. Where previously I had

$ verifyproblem.sh .
Loading problem myproblemid
Checking config
...
Checking submissions
ERROR in submissions: Require at least one "accepted" submission
   Slowest AC runtime: None, setting timelim to 300 secs, safety margin to 300 secs

I now get

$ verifyproblem.sh .
Loading problem myproblemid
Checking config
...
Checking submissions
[__main__.myproblemid.submission] ERROR: Require at least one
   "accepted" submission
   Slowest AC runtime: None, setting timelim to 300 secs, safety margin to 300 secs

I would prefer to revert the use of []-style log prefixes here, because this is user-facing UI and not logging that needs to be made systematic. But at the very least we should remove the mention of __main__ and avoid 80-col text wrapping.

@Tagl
Copy link
Contributor

Tagl commented Mar 31, 2024

I 100% agree with the text wrapping being worse now than previously. I do see great value in categorization of the log messages but I'd also prefer it to not look this way when running verifyproblem in default log level outputting to stdout.

@incaseoftrouble
Copy link
Contributor Author

incaseoftrouble commented Mar 31, 2024

[]-style log prefixes ... avoid 80-col text wrapping.

This is due to PlasTeX completely butchering the logging framework in a (near) irreparable way. Which is why I wanted to delay loading it.

I would prefer to revert the use of []-style log prefixes here, because this is user-facing UI and not logging that needs to be made systematic. But at the very least we should remove the mention of main

The correct "fix" for this is to remove eagerly importing plastex and, if needed, define a logging format that is applied in __main__. Then the output will look like before with the added bonus that problemtools' output can be configured when used as a library.

EDIT: I can do that if wanted!

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.

UnicodeDecodeError problems due to non-parseable _JUNK_CASES
4 participants