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

Use warnings.pm to calculate warnings tree #4

Merged
merged 4 commits into from Jan 27, 2014
Merged

Conversation

haarg
Copy link
Contributor

@haarg haarg commented Sep 5, 2013

This removes the hard coded warnings tree information, and instead uses the %Bits variable from warnings.pm to check what warnings categories belong to each other. This ensures that the categories are up to date with the perl version installed, and removes the need for Tree::DAG_Node as a prerequisite.

Although %Bits isn't a documented part of the warnings API, it has existed with the same structure since lexical warnings were introduced. This patch also doesn't depend any specific values in %Bits, so it should be safe across perl versions.

This also remove removes the $Test::Warn::Categorization:tree variable. Although it was documented, it was discouraged and was never used on CPAN.

@ribasushi
Copy link

+1. @haarg: excellent work!

@ribasushi
Copy link

Another reason to accept this pull request: Regardless of Tree::DAG_Node and its dependencies getting fxed to work on 5.8, Tree::DAG_Node irrevocably went 5.8.1 in order to support some unicode thingymajig. The rest of Test::Warn remains fully compatible with 5.6 however. This pull request will keep it that way.

Cheers

@haarg
Copy link
Contributor Author

haarg commented Nov 15, 2013

@chorny Did you ever get around to looking at this?

@ribasushi
Copy link

@chorny Poke poke. I am strongly urging you to look into releasing this: Tree::DAG_Node went all modern and depends on a lot of CPAN, making T::W rather very heavy. In addition it depends on utter crap like File::Slurp, which does not even install under HARNESS_OPTIONS=jX

Cheers

@ribasushi
Copy link

Added a related RT issue, sorry for the spam in advance, but the dependency on File::Slurp (among other things) continues being a problem, and there has been no response of any kind.

https://rt.cpan.org/Ticket/Display.html?id=92293

@chorny
Copy link
Owner

chorny commented Jan 25, 2014

Note: I'm not original author of this module, and there are places of this module which I don't know.

After studying code, I found out there is really no code for checking warnings in categories. There are no tests that check how Test::Warn works with categories. Check is done only for qr/category_name/. In some case this works, like for category 'uninitialized'. For 'utf8' it does not work. Perl does not have a list of warnings, so it is not possible to generate one for Test::Warn. It only can be done manually. It may be easier to write warning extractor for Perl source (did not studied yet if it is possible), but still it can't be fully automated.

So I see 2 possibilities:

  1. Drop warning categories from Test::Warn. I don't think that is a good solution.
  2. Add warnings to categories by request.

@karenetheridge
Copy link

On Sat, Jan 25, 2014 at 01:40:34PM -0800, Alexandr Ciornii wrote:

  1. Drop warning categories from Test::Warn. I don't think that is a good solution.

If they don't work in the first place, then at least this would be honest.

@haarg
Copy link
Contributor Author

haarg commented Jan 27, 2014

The warning category support is certainly limited and flawed, but many modules are already using it. Documenting its limitations would certainly be good, but I don't think removing it is reasonable.

This PR doesn't cause any problems that don't already exist. The existing warning tree baked into the module is just matching against the warning categories as well. Manually maintaining that list adds no value, because it is just a copy anyway.

So I still favor merging this, and probably adding a note about the limitations of the warning category support. Improving the category matching to work more accurately can be addressed separately.

chorny added a commit that referenced this pull request Jan 27, 2014
Use warnings.pm to calculate warnings tree
@chorny chorny merged commit 05ffe01 into chorny:master Jan 27, 2014
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.

None yet

4 participants