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 (updated) #42

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
4 participants
@kyzn
Contributor

kyzn commented Feb 22, 2017

Hi Dave! Please find attached pull request, that is based on #34. You had requested a few changes there, I went ahead and replaced eval with Try::Tiny and wrote some tests.

Let me know if there's anything I should update.
Thanks!

#cpan-prc

Improve level validation and canonicalisation logic
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
@preaction

This comment has been minimized.

Show comment
Hide comment
@preaction

preaction Feb 25, 2017

Contributor

Thanks for the patch! Of the changes @autarch mentioned needing in #34, the only thing that isn't quite right is that the Changes entry doesn't mention Kerin Millar's name for the patch (and now your name, too, for writing tests and cleaning it up). Once that is done, then this should be able to be merged.

Contributor

preaction commented Feb 25, 2017

Thanks for the patch! Of the changes @autarch mentioned needing in #34, the only thing that isn't quite right is that the Changes entry doesn't mention Kerin Millar's name for the patch (and now your name, too, for writing tests and cleaning it up). Once that is done, then this should be able to be merged.

Requested updates for improved level validation
- Make PerlTidy happy with no unnecessary string interpolation
- Update Changes
- Add tests
- Use Try::Tiny instead of eval
@kyzn

This comment has been minimized.

Show comment
Hide comment
@kyzn

kyzn Feb 25, 2017

Contributor

Hi @preaction, thanks for your comment! I updated Changes per your comment. Let me know if it looks good.

Contributor

kyzn commented Feb 25, 2017

Hi @preaction, thanks for your comment! I updated Changes per your comment. Let me know if it looks good.

@preaction

Excellent! 👍 from me!

@autarch

This comment has been minimized.

Show comment
Hide comment
@autarch

autarch Feb 25, 2017

Contributor

Merged from the CLI. Thanks!

Closes #34.

Contributor

autarch commented Feb 25, 2017

Merged from the CLI. Thanks!

Closes #34.

@autarch autarch closed this Feb 25, 2017

@kerframil

This comment has been minimized.

Show comment
Hide comment
@kerframil

kerframil Mar 3, 2017

Contributor

@kyzn Thanks for picking this up and rendering it fit for inclusion.

Contributor

kerframil commented Mar 3, 2017

@kyzn Thanks for picking this up and rendering it fit for inclusion.

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