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 level validation and canonicalisation logic #34

Closed
wants to merge 1 commit into from
Closed

Improve level validation and canonicalisation logic #34

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Dec 2, 2016

Having started working on RT bug #106495, I subsequently realised that the issue
had been resolved with GitHub PR #15. As I have a handful of modest improvements
left over from this work, I figured that I might as well submit them.

Note that this patch addresses the following discrepancy:-

# Assuming one log output where min_level = 0 ...
$dispatcher->is_debug()         # is true
$dispatcher->would_log('debug') # is true
$dispatcher->would_log(0)       # is false
  • Make level_is_valid() able to validate - and canonicalise - levels given as
    numbers, not just names
  • Remove redundant canonicalisation of numerical level from log()
  • In _level_as_number(), only return given level once validation has occurred
  • In _level_as_number(), following validation, only return the level as-is if it
    looks like an unsigned integer of any length [1]
  • In _level_as_name(), also check - and canonicalise - the given level with
    level_is_valid() [2]
  • In _level_as_name(), following validation, only return the level as-is if it
    does not look like an unsigned integer of any length [1]
  • Use ASCII-safe semantics (can't use /a modifier but [0-9] is safer than \d)
  • Where min_level/max_level are specified incorrectly, croak instead of dying
  • In _basic_init, use eval to suppress exceptions raised by _level_as_number(),
    so that we can continue to throw user-friendly exceptions where min_level or
    max_level are specified incorrectly

[1] DRY principle; validation having already been conducted by level_is_valid()
[2] Establishes symmetry with _level_as_number(), which also validates

Having started working on RT bug #106495, I subsequently realised that the issue
had been resolved with GitHub PR #15. As I have a handful of modest improvements
left over from this work, I figured that I might as well submit them.

Note that this patch addresses the following discrepancy:-

    # Assuming one log output where min_level = 0 ...
    $dispatcher->is_debug()         # is true
    $dispatcher->would_log('debug') # is true
    $dispatcher->would_log(0)       # is false

* Make level_is_valid() able to validate - and canonicalise - levels given as
  numbers, not just names
* Remove redundant canonicalisation of numerical level from log()
* In _level_as_number(), only return given level once validation has occurred
* In _level_as_number(), following validation, only return the level as-is if it
  looks like an unsigned integer of any length [1]
* In _level_as_name(), also check - and canonicalise - the given level with
  level_is_valid() [2]
* In _level_as_name(), following validation, only return the level as-is if it
  does not look like an unsigned integer of any length [1]
* Use ASCII-safe semantics (can't use /a modifier but [0-9] is safer than \d)
* Where min_level/max_level are specified incorrectly, croak instead of dying
* In _basic_init, use eval to suppress exceptions raised by _level_as_number(),
  so that we can continue to throw user-friendly exceptions where min_level or
  max_level are specified incorrectly

[1] DRY principle; validation having already been conducted by level_is_valid()
[2] Establishes symmetry with _level_as_number(), which also validates
Copy link
Member

@autarch autarch left a comment

Choose a reason for hiding this comment

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

This overall looks good. I have a few changes to request:

  1. I think this really needs some additional tests. It seems like you're fixing a bug with would_log, and at the very least that needs a test.
  2. Please use Try::Tiny instead of eval where you're using eval to catch exceptions from _level_as_number
  3. Please add an entry to Changes that references your name and this PR. See previous Changes entries for the format.
  4. If you're up for it, you might consider moving all of the level number/name conversion/checking logic to a separate module Log::Dispatch::Levels. This would provide exported subs rather than an OO interface. But this might be more work than you want to take on, so consider this optional.

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

Successfully merging this pull request may close these issues.

None yet

1 participant