-
Notifications
You must be signed in to change notification settings - Fork 5
feat: Add LogLevel enum with QUIET alias support #806
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
base: main
Are you sure you want to change the base?
Conversation
- Add LogLevel enum with DEBUG, INFO, WARNING, ERROR, CRITICAL, NONE, QUIET - QUIET is an enum alias for NONE (both resolve to 'NONE' value) - Add string 'QUIET' as valid logging_setting value (alias for 'NONE') - Update type hints to include 'QUIET' in AllowableLogLevels - Update Session and LogLevel docstrings with examples - Add 24 comprehensive tests in test_loglevel.py - Fix 4 broken tests in test_session.py (invalid log levels) - Maintain backward compatibility with string values - Improve developer experience with IDE autocomplete Benefits: - IDE autocomplete support via LogLevel.XXX constants - Matches industry conventions (git --quiet, pip --quiet) - Type safety while maintaining string backward compatibility - Clear self-documenting API
- Remove unused imports and variables - Fix comparison operators (use 'is' for True/None checks) - Format code with ruff - All tests still pass (51/51)
- Fix whitespace and line breaks in docstrings - Format AllowableLogLevels type hint for readability - Consistent quote style in string comparisons
After team discussion, decided to stick with NONE only. Changes: - Removed LogLevel.QUIET enum value - Removed 'QUIET' from AllowableLogLevels type hint - Removed 'QUIET' from str_to_log_level mapping - Updated Session docstring to remove QUIET references - Removed 5 QUIET-related tests (19 tests remain, down from 24) - Tests now validate that 'QUIET' is invalid All tests pass: 46/46 (27 session + 19 loglevel)
soulFood5632
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor comments around docstring and where some of the tests live.
| NONE: Suppress all logging output | ||
| Examples: | ||
| Using LogLevel constants (recommended - enables IDE autocomplete): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would get rid of the (recomonded line becuase lots of tools like vscode have autocomplete for literals as well
| f"logging_setting must be one of {list(str_to_log_level.keys())}, got {value}" | ||
| ) | ||
| self._logging_setting: AllowableLogLevels = value | ||
| self._logging_setting: AllowableLogLevels = str_value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit of nitpick, but typing wise the self._logging_setting can only be a string so we should be typing this properly here
| logging_setting (AllowableLogLevels, optional): The setting for the level of logging you would like to have. This will override the module-level logging settings for the duration of this session. | ||
| logging_setting (LogLevel | str, optional): Controls log verbosity. This will override the module-level logging settings for the duration of this session. | ||
| Use LogLevel constants (recommended - enables IDE autocomplete): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above, get rid of the line "(recomonded - enables IDE autocomplete)"
| # ================ END Edge Cases =============== | ||
|
|
||
|
|
||
| if __name__ == "__main__": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would delete this line for consitency sake
| # ================ END Documentation Tests =============== | ||
|
|
||
|
|
||
| # ================= START Integration Tests =============== |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would move this test to either the Session unit tests or integration folder
| assert str(rt.LogLevel.NONE) == "NONE" | ||
|
|
||
|
|
||
| def test_session_with_none_logging_uses_global_config(mock_session_dependencies): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here.
What does this add?
This PR adds support for
"QUIET"as a valid string alias for thelogging_settingparameter, complementing the existingLogLevel.QUIETenum. This improves developer experience by matching industry conventions (likegit --quiet,pip --quiet) while maintaining full backward compatibility.Problem: Users expected
logging_setting="QUIET"to work based on industry conventions, but only"NONE"was accepted as a string, causing confusion.Solution: Added
"QUIET"to the allowable string values, mapping to the same log level as"NONE". Both the enum (LogLevel.QUIET) and string ("QUIET") now work seamlessly.Type of changes
Please check the type of change your PR introduces:
Background context
The
LogLevelenum was introduced to provide IDE autocomplete support and better developer experience. However, only the enumLogLevel.QUIETworked, not the string"QUIET". This created an inconsistency where users familiar with Unix tools (--quietflag) couldn't use the string form they expected.This PR resolves issue #805 by adding string
"QUIET"support alongside the enum.Checklist for Author
Code Quality
ruff check .andruff format .)Testing
pytest tests)Documentation
Git & PR Management
Breaking Changes
Final Product
Usage Examples
All of these methods now work to suppress logs:
What Changed
"QUIET"LogLevel.QUIETLiteral["DEBUG", "INFO", ..., "NONE"]Literal["DEBUG", "INFO", ..., "NONE", "QUIET"]